-
Notifications
You must be signed in to change notification settings - Fork 277
[GeoMechanicsApplication] More improvements for the nodal extrapolation process #14093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GeoMechanicsApplication] More improvements for the nodal extrapolation process #14093
Conversation
- The extrapolation process now keeps its own map from node ID to a set of element IDs rather than an element counter that is stored with the nodes themselves. In this way, we avoid interference of model parts that share nodes and/or multiple extrapolation processes per stage. - Removed member `ExecuteFinalize`, since we no longer need to reset the element counters that were kept by the nodes. - Two nodes were added to a different model part than the intended one. That has been corrected now. - Made the error messages a little bit more descriptive when the unit tests fail. - Use `EXPECT_NEAR` rather than `KRATOS_EXPECT_NEAR`. - In the unit test that uses two extrapolation processes, each process has its own extrapolation variable. - Removed a unit test that covered an implementation detail of the extrapolation process.
…anicsApplication's code base
In this way, we no longer need a loop counter in addition to the loop variable.
Also added some member functions that return the zero value of the given variable (e.g. a scalar, vector, or matrix).
Note that we have to be explicit about the type (`const T`), since using `auto` yields a type with lazy evaluation resulting in test failures. (Or, more accurately, that's what I expect is going on.)
…delPart from .mdpa Used model_part_name_list to reflect active modelparts for extrapolation to nodes. Stripped superfluous retention law parameters from MaterialParameters.json.
…ddition and the counting check if the element is active.
It now says that the second element in the test is inactive. For that test case, we now also use the default absolute tolerance for testing.
One unit test now uses this new utility function.
…or of arrays Also resolved a few ambiguous function calls.
Also extracted a function to assert matrices at a collection of nodes.
WPK4FEM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the massive clean up of the unit test for the extrapolation process. I have some remarks, but none of them is blocking.
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Outdated
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Show resolved
Hide resolved
- Added a simple sketch of the model created by `CreateModelPartWithTwoStubElements`. - Renamed a local helper function.
…ts-for-nodal-extrapolation-process
WPK4FEM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates.
rfaasse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for these bugfixes, clean-up and reduction of duplications in the unit tests! I have some questions about the behavior and a couple of suggestions, but all in all a very neat PR
...nicsApplication/custom_processes/geo_extrapolate_integration_point_values_to_nodes_process.h
Show resolved
Hide resolved
...nicsApplication/custom_processes/geo_extrapolate_integration_point_values_to_nodes_process.h
Show resolved
Hide resolved
applications/GeoMechanicsApplication/test_setup_utilities/model_setup_utilities.cpp
Show resolved
Hide resolved
...cations/GeoMechanicsApplication/tests/test_settlement_workflow/ProjectParameters_stage4.json
Show resolved
Hide resolved
...cations/GeoMechanicsApplication/tests/test_settlement_workflow/ProjectParameters_stage3.json
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Outdated
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Outdated
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Outdated
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Outdated
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Outdated
Show resolved
Hide resolved
- Added `override`s of member `SetValuesOnIntegrationPoints` of the stub element. - Extracted a helper function that asserts nodal vector values (to reduce duplication).
- Added `override`s of member `SetValuesOnIntegrationPoints` of the stub element. - Extracted a helper function that asserts nodal vector values (to reduce duplication). - Removed a redundant unit test. - Changed the integration point values set on the integration points of the inactive element (to be more confident that inactive elements are not taken into account). - For two unit tests, used slightly more complicated fields to be extrapolated. Also added two comments that explain the expected nodal values. - Updated the documentation of the nodal extrapolation process.
…ts-for-nodal-extrapolation-process
rfaasse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for processing the review comments! It's good to go from my side
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Show resolved
Hide resolved
...cations/GeoMechanicsApplication/tests/test_settlement_workflow/ProjectParameters_stage4.json
Show resolved
Hide resolved
...cations/GeoMechanicsApplication/tests/test_settlement_workflow/ProjectParameters_stage3.json
Show resolved
Hide resolved
...nicsApplication/custom_processes/geo_extrapolate_integration_point_values_to_nodes_process.h
Show resolved
Hide resolved
...csApplication/custom_processes/geo_extrapolate_integration_point_values_to_nodes_process.cpp
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Outdated
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Outdated
Show resolved
Hide resolved
markelov208
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Anne, very nice improvements and, especially, helpers for the unit tests. I have only minor comments. Would you mind to create an issue to generalized the tests for other elements/interfaces?
...csApplication/custom_processes/geo_extrapolate_integration_point_values_to_nodes_process.cpp
Outdated
Show resolved
Hide resolved
...pp_tests/custom_processes/test_geo_extrapolate_integration_point_values_to_nodes_process.cpp
Outdated
Show resolved
Hide resolved
- Renamed a private member function to better reflect what it does. - Renamed a local variable to make it more consistent with other variable names.
e4160af to
c564646
Compare
WPK4FEM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ready for me.
markelov208
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Anne, thank you for processing the comments. The PR is ready to be merged
📝 Description
Fixed some bugs that were found while testing the nodal extrapolation process.
🆕 Changelog
ExecuteFinalizehad become redundant and is now removed.forloop by a call tostd::inner_product. This change required the introduction of member functions that return the zero value of the variable to be extrapolated.