[GeoMechanicsApplication] Reduce duplicated functionality between schemes and refactor the inheritance structure #11808

rfaasse · 2023-11-15T10:35:40Z

📝 Description
This PR aims to simplify the class structure of the schemes in the geomechanics application. This reduces the number of duplicated functionality and makes the structure more intuitive. In an earlier PR, the common functionality was extracted from the UPw scheme to the geomechanics time integration scheme, which is still the baseclass of our hierarchy. In this PR, base classes for BackwardEuler and GeneralizedNewmark schemes were created which contains al common functionality in a generalized way. This makes it a lot easier to add a new scheme with different scalar variables.

Note that not all UPw functionality is cleaned up in the same way (the backward euler upw scheme still incorrectly derives from the newmark scheme). However, when changing that part as well, the overview of what is happening in this refactoring effort might be lost. Hence, it's not included in this PR but will be part of future efforts.

Before refactoring, unit tests were put in place to make sure the functionality didn't change along the way (see the test folder in this PR)

Old class hierarchy depiction:

New class hierarchy depiction:

…s or conditions

…process

… geo/re-use-functionality-between-schemes

…process

… geo/re-use-functionality-between-schemes

…rature scheme

applications/GeoMechanicsApplication/custom_strategies/schemes/backward_euler_scheme.hpp

applications/GeoMechanicsApplication/custom_strategies/schemes/generalized_newmark_scheme.hpp

...s/GeoMechanicsApplication/custom_strategies/schemes/geomechanics_time_integration_scheme.hpp

applications/GeoMechanicsApplication/custom_strategies/schemes/newmark_dynamic_U_Pw_scheme.hpp

applications/GeoMechanicsApplication/tests/cpp_tests/test_generalized_newmark_T_scheme.cpp

applications/GeoMechanicsApplication/tests/cpp_tests/test_newmark_Pw_scheme.cpp

WPK4FEM · 2023-11-20T13:37:36Z

applications/GeoMechanicsApplication/tests/test_generalized_newmark_scheme.cpp

+
+    KRATOS_EXPECT_EXCEPTION_IS_THROWN(
+        SchemeType scheme(invalid_theta, TEMPERATURE, DT_TEMPERATURE, DT_TEMPERATURE_COEFFICIENT),
+        "Theta must be larger than zero, but got -2")


0<= Theta <= 1 ?

I took the original restrictions here, since if theta = 0.0, we will end up with a division by 0. Is there a good reason to have 1.0 as the upper limit (same would hold then for beta and/or gamma)?

rfaasse

Thanks for the review @WPK4FEM, implemented most of the suggestions (some were a bit out of scope in my opinion, so let me know what you think)

applications/GeoMechanicsApplication/custom_strategies/schemes/newmark_dynamic_U_Pw_scheme.hpp

rfaasse · 2023-11-20T16:02:22Z

...ations/GeoMechanicsApplication/custom_strategies/schemes/newmark_quasistatic_U_Pw_scheme.hpp

+    using BaseType = Scheme<TSparseSpace, TDenseSpace>;
+    using DofsArrayType = typename BaseType::DofsArrayType;
+    using TSystemMatrixType = typename BaseType::TSystemMatrixType;
+    using TSystemVectorType = typename BaseType::TSystemVectorType;


What I did here, is format the file using the clang-format settings that are found in the kratos wiki. However, since it's not in the style guide, I think we can still change this if we would want to

rfaasse · 2023-11-21T14:27:57Z

...ations/GeoMechanicsApplication/custom_strategies/schemes/newmark_quasistatic_U_Pw_scheme.hpp

+    void FinalizeSolutionStep(ModelPart& rModelPart,
+                              TSystemMatrixType& A,
+                              TSystemVectorType& Dx,
+                              TSystemVectorType& b) override
    {
        KRATOS_TRY

        if (rModelPart.GetProcessInfo()[NODAL_SMOOTHING]) {


I agree we should find a more logical place for it in the future

...ations/GeoMechanicsApplication/custom_strategies/schemes/newmark_quasistatic_U_Pw_scheme.hpp

...GeoMechanicsApplication/custom_strategies/schemes/newmark_quasistatic_damped_U_Pw_scheme.hpp

rfaasse · 2023-11-21T14:57:15Z

...ations/GeoMechanicsApplication/custom_strategies/schemes/newmark_quasistatic_U_Pw_scheme.hpp

+            noalias(rNode.FastGetSolutionStepValue(ACCELERATION)) =
+                ((rNode.FastGetSolutionStepValue(DISPLACEMENT) -
+                  rNode.FastGetSolutionStepValue(DISPLACEMENT, 1)) -
+                 this->GetDeltaTime() * rNode.FastGetSolutionStepValue(VELOCITY, 1) -
+                 (0.5 - mBeta) * this->GetDeltaTime() * this->GetDeltaTime() *
+                     rNode.FastGetSolutionStepValue(ACCELERATION, 1)) /
+                (mBeta * this->GetDeltaTime() * this->GetDeltaTime());
+
+            noalias(rNode.FastGetSolutionStepValue(VELOCITY)) =
+                rNode.FastGetSolutionStepValue(VELOCITY, 1) +
+                (1.0 - mGamma) * this->GetDeltaTime() *
+                    rNode.FastGetSolutionStepValue(ACCELERATION, 1) +
+                mGamma * this->GetDeltaTime() * rNode.FastGetSolutionStepValue(ACCELERATION);


Nice suggestion!

...s/GeoMechanicsApplication/custom_strategies/schemes/geomechanics_time_integration_scheme.hpp

applications/GeoMechanicsApplication/custom_strategies/schemes/generalized_newmark_scheme.hpp

applications/GeoMechanicsApplication/custom_strategies/schemes/backward_euler_scheme.hpp

…e.hpp purely virtual

avdg81

This is a very nice step forwards again. Thank you very much for the hard work that you have done Richard! The structure has improved a lot and it makes much more sense now.

I only have several minor remarks, nothing blocking.

...s/GeoMechanicsApplication/custom_strategies/schemes/geomechanics_time_integration_scheme.hpp

avdg81 · 2023-11-21T13:10:15Z

...s/GeoMechanicsApplication/custom_strategies/schemes/geomechanics_time_integration_scheme.hpp

+    {
+        KRATOS_TRY
+
+        Scheme<TSparseSpace, TDenseSpace>::Check(rModelPart);


We can make this slightly more readable:

Suggested change

Scheme<TSparseSpace, TDenseSpace>::Check(rModelPart);

BaseType::Check(rModelPart);

I actually prefer to keep it like this: now it is clear at a glance (without having to look at the definition) which function is called. We could discuss (we could also delete the base check, since at this moment it just returns 0 and funnily enough has a KRATOS_TRY and CATCH around it).

...s/GeoMechanicsApplication/custom_strategies/schemes/geomechanics_time_integration_scheme.hpp

avdg81 · 2023-11-22T10:02:22Z

applications/GeoMechanicsApplication/tests/cpp_tests/test_backward_euler_Pw_scheme.cpp

+KRATOS_TEST_CASE_IN_SUITE(ForMissingNodalDof_CheckBackwardEulerPwScheme_Throws,
+                          KratosGeoMechanicsFastSuite)


Just a remark: apparently, the other tests don't suffer from the fact that no water pressure degree of freedom was added to the node. It's just that when Check is called and the DoF is missing, it will throw an exception. That surprises me a bit.

Indeed, if check isn't called, it won't throw (at least when the DoF is not used in the function that's tested in that particular test). I hope everywhere these schemes are used, the check is actually done, but I don't know. I'm not sure how easy it would be to change this way of checking (apart from just doing the check at the start of every public function, which I don't know if I like)

avdg81 · 2023-11-22T10:21:00Z

applications/GeoMechanicsApplication/tests/cpp_tests/test_backward_euler_T_scheme.cpp

+
+    Model model;
+    auto& model_part = model.CreateModelPart("dummy", 2);
+    auto p_node = model_part.CreateNewNode(0, 0.0, 0.0, 0.0);


You may want to add variable DT_TEMPERATURE (to be consistent with test case ForMissingWaterPressureSolutionStepVariable_CheckBackwardEulerPwScheme_Throws):

Suggested change

auto p_node = model_part.CreateNewNode(0, 0.0, 0.0, 0.0);

model_part.AddNodalSolutionStepVariable(DT_TEMPERATURE);

auto p_node = model_part.CreateNewNode(0, 0.0, 0.0, 0.0);

I removed this line due to the review comment of WPK, which I agree with. I prefer to remove the same line from the Pw version (and the newmark T/Pw equivalents)

applications/GeoMechanicsApplication/tests/cpp_tests/test_newmark_Pw_scheme.cpp

avdg81 · 2023-11-22T10:42:19Z

applications/GeoMechanicsApplication/tests/cpp_tests/test_newmark_quasistatic_U_Pw_scheme.cpp

@@ -42,7 +42,6 @@ class NewmarkQuasistaticUPwSchemeTester {
    void CreateValidModelPart()
    {
        auto& result = mModel.CreateModelPart("dummy", 2);
-        result.AddNodalSolutionStepVariable(TEMPERATURE);


Good catch!

rfaasse

Thanks both @avdg81 and @WPK4FEM for the thorough reviews. I processed the comments, so should be ready for re-review!

rfaasse · 2023-11-22T13:26:18Z

applications/GeoMechanicsApplication/custom_strategies/schemes/generalized_newmark_scheme.hpp

+class GeneralizedNewmarkScheme
+    : public GeoMechanicsTimeIntegrationScheme<TSparseSpace, TDenseSpace> {
+public:
+    explicit GeneralizedNewmarkScheme(double theta,


Agreed, remnant from when the only parameter was theta

rfaasse · 2023-11-22T13:31:10Z

...s/GeoMechanicsApplication/custom_strategies/schemes/geomechanics_time_integration_scheme.hpp

Good suggestion, made it uniform across the schemes that implement these functions

rfaasse · 2023-11-22T13:33:54Z

...s/GeoMechanicsApplication/custom_strategies/schemes/geomechanics_time_integration_scheme.hpp

+    {
+        KRATOS_TRY
+
+        Scheme<TSparseSpace, TDenseSpace>::Check(rModelPart);


I actually prefer to keep it like this: now it is clear at a glance (without having to look at the definition) which function is called. We could discuss (we could also delete the base check, since at this moment it just returns 0 and funnily enough has a KRATOS_TRY and CATCH around it).

...s/GeoMechanicsApplication/custom_strategies/schemes/geomechanics_time_integration_scheme.hpp

applications/GeoMechanicsApplication/tests/cpp_tests/test_backward_euler_Pw_scheme.cpp

rfaasse · 2023-11-22T13:46:24Z

applications/GeoMechanicsApplication/tests/cpp_tests/test_backward_euler_Pw_scheme.cpp

+KRATOS_TEST_CASE_IN_SUITE(ForMissingNodalDof_CheckBackwardEulerPwScheme_Throws,
+                          KratosGeoMechanicsFastSuite)


Indeed, if check isn't called, it won't throw (at least when the DoF is not used in the function that's tested in that particular test). I hope everywhere these schemes are used, the check is actually done, but I don't know. I'm not sure how easy it would be to change this way of checking (apart from just doing the check at the start of every public function, which I don't know if I like)

rfaasse · 2023-11-22T13:49:39Z

applications/GeoMechanicsApplication/tests/cpp_tests/test_backward_euler_T_scheme.cpp

+
+    Model model;
+    auto& model_part = model.CreateModelPart("dummy", 2);
+    auto p_node = model_part.CreateNewNode(0, 0.0, 0.0, 0.0);


I removed this line due to the review comment of WPK, which I agree with. I prefer to remove the same line from the Pw version (and the newmark T/Pw equivalents)

applications/GeoMechanicsApplication/tests/cpp_tests/test_newmark_Pw_scheme.cpp

WPK4FEM

Read through once more. Looks like a consistent set of things to me.

rfaasse and others added 30 commits November 6, 2023 17:23

Added schemes and first unit-tests

f41f3ab

Added unit test for temperature specific functions

dbca215

Moved unit test and added new tests for functions that act on element…

44b0a64

…s or conditions

Added a test for the initialize scheme function

9be9b86

Added a test for the predict scheme function

0b69dec

Added tests for GetDofList and EquationId for the elements/conditions

d026db6

Extract geo mechanics scheme

782aafc

Clean up the backward euler quasistatic scheme

75b2304

Minor clean-up

9e4c2a3

Removed code smell related to protected member variable

9827044

Fix For GCC Build

8ae7f5b

Solved implicit searching for protected functions

d90af09

Moved relevant unit tests to test_geo_mechanics_scheme.cpp + cleaning up

a5b5223

Minor fix, override instead of virtual function

e6505c3

Partly Removing SQ issues

38639a9

Merge branch 'master' into geo/11769-add-new-schemes-for-the-thermal-…

6390760

…process

Minor fix

312d525

Register the new schemes to python

f55e41b

Renamed classes as per Wijtze Pieters suggestion

3a8cb85

Minor change to not repeat functionality in the test

a82da19

Refactored schemes

5b85a0a

Fix build issue for GCC

78cf7a3

Processing review comments #1

c556029

Merge branch 'geo/11769-add-new-schemes-for-the-thermal-process' into…

8195832

… geo/re-use-functionality-between-schemes

Processing review comments #2

bdb26ac

Merge branch 'master' into geo/11769-add-new-schemes-for-the-thermal-…

c33c1f0

…process

Merge branch 'geo/11769-add-new-schemes-for-the-thermal-process' into…

f1ea935

… geo/re-use-functionality-between-schemes

Removed inheritance coupling between newmark and backward euler tempe…

5d93005

…rature scheme

Added some more tests for the other schemes

4f12b98

Split out backward_euler_PW scheme from newmark

6ef8e9c

rfaasse added 8 commits November 17, 2023 12:43

Fix UTs after bringing branch up to date with master

c170e62

Fix pipeline build after merge

2b3b9f8

Processing review comments from previous PR

b532878

Removed throwing error since it broke tests

086a745

Revert rename

27bc6a9

Removed duplicate tests

7305909

Merge branch 'master' into geo/re-use-functionality-between-schemes

d9ebc18

Clean up some includes

2cbdb70

rfaasse marked this pull request as ready for review November 20, 2023 12:10

rfaasse added the GeoMechanics Issues related to the GeoMechanicsApplication label Nov 20, 2023

rfaasse requested review from avdg81 and WPK4FEM November 20, 2023 12:16

rfaasse changed the title ~~[GeoMechanicsApplication] Geo/re use functionality between schemes~~ [GeoMechanicsApplication] Reduce duplicated functionality between schemes and refactor the inheritance structure Nov 20, 2023

WPK4FEM reviewed Nov 20, 2023

View reviewed changes

rfaasse added 3 commits November 21, 2023 11:28

Add test for Update

91e8790

Changed ancient parallelization method to block_for_each

d2c6923

Cleaning up the checking mechanism

a70d0a8

rfaasse linked an issue Nov 21, 2023 that may be closed by this pull request

[GeoMechanicsApplication] Improve inheritance structure of thermal and related schemes #11805

Closed

rfaasse added 2 commits November 21, 2023 16:09

Processing review comments WPK

ee2e68e

Fixing some last SQ issues

4e2030c

rfaasse commented Nov 21, 2023

View reviewed changes

Made not-implemented functions in geomechanics_time_integration_schem…

103e678

…e.hpp purely virtual

avdg81 reviewed Nov 22, 2023

View reviewed changes

Processing review comments AvdG

3364720

rfaasse commented Nov 22, 2023

View reviewed changes

rfaasse requested review from avdg81 and WPK4FEM November 22, 2023 14:13

WPK4FEM approved these changes Nov 22, 2023

View reviewed changes

rfaasse merged commit 2bd73d7 into master Nov 22, 2023
15 checks passed

rfaasse deleted the geo/re-use-functionality-between-schemes branch November 22, 2023 16:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GeoMechanicsApplication] Reduce duplicated functionality between schemes and refactor the inheritance structure #11808

[GeoMechanicsApplication] Reduce duplicated functionality between schemes and refactor the inheritance structure #11808

rfaasse commented Nov 15, 2023 •

edited

Loading

WPK4FEM Nov 20, 2023

rfaasse Nov 21, 2023

rfaasse left a comment

rfaasse Nov 20, 2023

rfaasse Nov 21, 2023

rfaasse Nov 21, 2023

avdg81 left a comment

avdg81 Nov 21, 2023

rfaasse Nov 22, 2023

avdg81 Nov 22, 2023

rfaasse Nov 22, 2023

avdg81 Nov 22, 2023

rfaasse Nov 22, 2023

avdg81 Nov 22, 2023

rfaasse left a comment

rfaasse Nov 22, 2023

rfaasse Nov 22, 2023

rfaasse Nov 22, 2023

rfaasse Nov 22, 2023

rfaasse Nov 22, 2023

WPK4FEM left a comment

	Scheme<TSparseSpace, TDenseSpace>::Check(rModelPart);
	BaseType::Check(rModelPart);

		KRATOS_TEST_CASE_IN_SUITE(ForMissingNodalDof_CheckBackwardEulerPwScheme_Throws,
		KratosGeoMechanicsFastSuite)

	auto p_node = model_part.CreateNewNode(0, 0.0, 0.0, 0.0);
	model_part.AddNodalSolutionStepVariable(DT_TEMPERATURE);
	auto p_node = model_part.CreateNewNode(0, 0.0, 0.0, 0.0);

[GeoMechanicsApplication] Reduce duplicated functionality between schemes and refactor the inheritance structure #11808

[GeoMechanicsApplication] Reduce duplicated functionality between schemes and refactor the inheritance structure #11808

Conversation

rfaasse commented Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfaasse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avdg81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfaasse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WPK4FEM left a comment

Choose a reason for hiding this comment

rfaasse commented Nov 15, 2023 •

edited

Loading