[GeoMechanicsApplication] Extend class LineInterfaceElement such that it also includes planar interface elements#13634
Conversation
# Conflicts: # applications/GeoMechanicsApplication/custom_elements/interface_element.cpp # applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
rfaasse
left a comment
There was a problem hiding this comment.
Thank you for adding this well-tested extension of the interface element. I have some suggestions, but nothing major
applications/GeoMechanicsApplication/custom_elements/interface_element.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_utilities/geometry_utilities.cpp
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
Show resolved
Hide resolved
applications/GeoMechanicsApplication/custom_elements/interface_element.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
Show resolved
Hide resolved
avdg81
left a comment
There was a problem hiding this comment.
Hi Gennady,
Thanks a lot for generalizing the line interface element to an interface element that supports both line and surface geometry. I'm also happy to see that you've added lots of unit tests. Well done! I have several suggestions, but they're all minor. We may want to revisit the test code at some point, but for now this looks good enough to me. Let's try to get this in as soon as possible.
| : Element(NewId, rGeometry, rProperties), | ||
| mIntegrationScheme(std::make_unique<LobattoIntegrationScheme>(GetGeometry().PointsNumber() / 2)), | ||
| mStressStatePolicy(std::make_unique<Line2DInterfaceStressState>()) | ||
| InterfaceElement::InterfaceElement(IndexType NewId, |
There was a problem hiding this comment.
I'm just wondering whether we should name this class GeoInterfaceElement to avoid future name clashes with other applications? Alternatively, we could consider to wrap this class in a Geo namespace. I'm curious how you and @rfaasse feel about this?
There was a problem hiding this comment.
Agreed. Currently, it is not changed.
| InterfaceElement::InterfaceElement(IndexType NewId, | ||
| const GeometryType::Pointer& rGeometry, | ||
| std::unique_ptr<StressStatePolicy> pStressStatePolicy) | ||
| : Element(NewId, rGeometry), mpStressStatePolicy(std::move(pStressStatePolicy)) |
There was a problem hiding this comment.
Just something to consider: perhaps we can use forwarding constructors to avoid some code duplication?
There was a problem hiding this comment.
Sorry I've heard about the forwarding constructors but AI does not help. Perhaps, we can discuss it tomorrow.
applications/GeoMechanicsApplication/custom_elements/interface_element.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
Outdated
Show resolved
Hide resolved
| template <typename TElementFactory> | ||
| InterfaceElement CreateAndInitializeElement(TElementFactory&& rFactory, | ||
| const Properties::Pointer& rProperties, | ||
| const std::vector<std::pair<std::size_t, array_1d<double, 3>>>& rDisplacements = {}) |
There was a problem hiding this comment.
I find the first function parameter hard to read (rvalue reference). When I look at its usages, my understanding is that you'd like to pass a function object that builds an interface element given the model and the desired properties. Perhaps removing the && already helps?
I'm also a bit worried about the model instance here, since model will go out of scope as soon as this function returns. Any other objects that keep a pointer or reference to that model, will then have dangling pointers and references, which is very dangerous and hard to debug.
I've discussed this with @rfaasse and we agreed that we can look into that later, in order to not delay merging this PR.
There was a problem hiding this comment.
Now, removed &&. It still works ;)
applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
Outdated
Show resolved
Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
Outdated
Show resolved
Hide resolved
| const auto p_geometry = std::make_shared<TriangleInterfaceGeometry3D3Plus3Noded>(nodes); | ||
| const InterfaceElement element(0, p_geometry, p_properties, | ||
| std::make_unique<Line2DInterfaceStressState>()); |
There was a problem hiding this comment.
Also here I'm confused. We're creating a surface interface element (that's what the geometry suggests), but then we pass it a stress state instance that is intended for 2D line interfaces. Perhaps we should not allow that? At least, we shouldn't showcase that in a unit test, in my opinion.
There was a problem hiding this comment.
Oh, another mistake. I think we should not allow. Perhaps, a space dimension shall be added to the StressStatePolicy then we can check a correspondence of the geometry and the stress state.
applications/GeoMechanicsApplication/tests/cpp_tests/custom_elements/test_interface_element.cpp
Outdated
Show resolved
Hide resolved
markelov208
left a comment
There was a problem hiding this comment.
Hi Richard and Anne, thank you very much for the reviews. Not everything is changed, hope we can discuss it.
avdg81
left a comment
There was a problem hiding this comment.
Hi Gennady, thanks for processing the review suggestions so swiftly. I feel we're almost there. I have one more question about the function that calculates the expected LHS matrices.
| Matrix CreateExpectedLeftHandSideForTriangleElement(std::size_t dimension, | ||
| double NormalStiffness, | ||
| double ShearStiffness, | ||
| double divider1 = 3.0, | ||
| double divider2 = 3.0) | ||
| { | ||
| Matrix expected_left_hand_side = ZeroMatrix(dimension, dimension); | ||
| double value; | ||
| std::size_t counter = 0; | ||
| std::size_t half_dimension = dimension / 2; | ||
| for (std::size_t i = 0; i < half_dimension; i++) { | ||
| if (++counter == 3) { | ||
| value = NormalStiffness; | ||
| counter = 0; | ||
| } else { | ||
| value = ShearStiffness; | ||
| } | ||
| if (i < 9) { | ||
| value /= divider1; | ||
| } else { | ||
| value /= divider2; | ||
| } | ||
| expected_left_hand_side(i, i) = value; | ||
| expected_left_hand_side(i, i + half_dimension) = -value; | ||
| expected_left_hand_side(i + half_dimension, i) = -value; | ||
| expected_left_hand_side(i + half_dimension, i + half_dimension) = value; | ||
| } | ||
|
|
||
| return expected_left_hand_side; | ||
| } |
There was a problem hiding this comment.
I understand that you'd like to reuse one or a few of the expected left hand side matrices, but I'm not sure whether this is the way to go. I don't understand the implementation of this function. How can I check that this makes sense from a physics/FEM point of view? My understanding of @rfaasse's suggestion was to extract a function that returns a matrix with hard-coded numbers, since some of these matrices were repeated.
If we'd like to keep the current implementation, I feel that we need a comment that explains the calculation, perhaps with some references to finite element books/papers?
There was a problem hiding this comment.
Thank you for the clarification. Now there is a function that provide these values. I put a comment that the values are taken from the element. The theory, it looks 3D case with matrix of 18x18 is consistent with 2D case 8x8 but 3D case of 36x36 is something special.
| const auto prescribed_displacements = PrescribedDisplacements{ | ||
| {2, array_1d<double, 3>{0.2, 0.5, 0.0}}, {3, array_1d<double, 3>{0.2, 0.5, 0.0}}}; | ||
| auto element = CreateAndInitializeElement(CreateHorizontalUnitLength2Plus2NodedLineInterfaceElementWithUDofs, | ||
| p_properties, prescribed_displacements); |
rfaasse
left a comment
There was a problem hiding this comment.
Thanks for processing the review comments, to me this looks ready to go!
📝 Description
A brief description of the PR.
Please mark the PR with appropriate tags: