[GeoMechanicsApplication] Clean up process for applying constant scalar values by rfaasse · Pull Request #13676 · KratosMultiphysics/Kratos

rfaasse · 2025-07-25T13:37:28Z

📝 Description
This PR reduces code smells and simplifies the code

smell

…forward declares

rfaasse · 2025-07-25T13:38:49Z

applications/GeoMechanicsApplication/custom_workflows/dgeoflow.cpp

+    mProcesses.push_back(make_shared<GeoApplyConstantScalarValueProcess>(part, Parameters{R"(
+        {
+            "variable_name"  : "VOLUME_ACCELERATION_X",
+            "value"          : 0.0,
+            "is_fixed"       : true
+        })"}));
+
+    mProcesses.push_back(make_shared<GeoApplyConstantScalarValueProcess>(part, Parameters{R"(
+        {
+            "variable_name"  : "VOLUME_ACCELERATION_Y",
+            "value"          : -9.81,
+            "is_fixed"       : true
+        })"}));
+
+    mProcesses.push_back(make_shared<GeoApplyConstantScalarValueProcess>(part, Parameters{R"(
+        {
+            "variable_name"  : "VOLUME_ACCELERATION_Z",
+            "value"          : 0.0,
+            "is_fixed"       : true
+        })"}));


I think we should look at this (later), it seems strange to me that we hard-code it, but for now I kept it as it is, just using the 'parameters' version of the constructor

Agreed. In this way, we are also hard-coding the gravity direction and the value of the gravity acceleration (assuming all input is given in SI).

avdg81

Hi Richard, thanks a lot for cleaning up this process and getting rid of several code smells. I see that understanding the process has become simpler due to your changes. And having a few more unit tests is also a very nice extension.

I have several minor suggestions and a few questions. Hope this helps.

avdg81 · 2025-07-30T07:40:36Z

...cations/GeoMechanicsApplication/custom_processes/geo_apply_constant_scalar_value_process.cpp

-GeoApplyConstantScalarValueProcess::GeoApplyConstantScalarValueProcess(ModelPart& rModelPart, Parameters ThisParameters)
-    : Process(Flags()), mrModelPart(rModelPart)
+GeoApplyConstantScalarValueProcess::GeoApplyConstantScalarValueProcess(ModelPart& rModelPart,
+                                                                       const Parameters& ThisParameters)


Just to comply with the Kratos Style Guide:

Suggested change

const Parameters& ThisParameters)

const Parameters& rParameters)

avdg81 · 2025-07-30T07:45:38Z

...cations/GeoMechanicsApplication/custom_processes/geo_apply_constant_scalar_value_process.cpp


    mVariableName = ThisParameters["variable_name"].GetString();
-    this->Set(VARIABLE_IS_FIXED, ThisParameters["is_fixed"].GetBool());
+    mIsFixed      = ThisParameters.Has("is_fixed") ? ThisParameters["is_fixed"].GetBool() : false;


Does this change mean that parameter "is_fixed" is no longer required, but optional (having a default value of false if not provided)? If yes, could you please help me understand why this change is needed?

Also, if this functional change is needed, we are repeating the default value when "is_fixed" is not provided (the default value of mIsFixed is also set in the header file, which is great). So perhaps we can rewrite this statement to:

Suggested change

mIsFixed = ThisParameters.Has("is_fixed") ? ThisParameters["is_fixed"].GetBool() : false;

if (ThisParameters.Has("is_fixed")) mIsFixed = ThisParameters["is_fixed"].GetBool();

As discussed, it was handled by using the default parameters before, so the behavior hasn't changed. I implemented your suggestion, because indeed the default is already there in the header.

avdg81 · 2025-07-30T07:52:26Z

...cations/GeoMechanicsApplication/custom_processes/geo_apply_constant_scalar_value_process.cpp

+        << "' which is not of type Variable<double>.\n";
+
+    KRATOS_ERROR_IF_NOT(rModelPart.GetNodalSolutionStepVariablesList().Has(KratosComponents<VariableData>::Get(mVariableName)))
+        << "Trying to fix a variable that is not in the rModelPart - variable name is "


Perhaps we can make the error message a little bit more specific by reporting the name of the model part rather than rModelPart?

avdg81 · 2025-07-30T08:01:04Z

...cations/GeoMechanicsApplication/custom_processes/geo_apply_constant_scalar_value_process.cpp

+    if (mIsFixed && KratosComponents<Variable<double>>::Has(mVariableName)) {
+        VariableUtils().ApplyFixity(KratosComponents<Variable<double>>::Get(mVariableName), true,
+                                    mrModelPart.Nodes());
+    }


I don't understand why we shouldn't need to call ApplyFixity when mIsFixed equals false. In other words, why wouldn't we write this:

if (KratosComponents<Variable<double>>::Has(mVariableName)) { VariableUtils().ApplyFixity(KratosComponents<Variable<double>>::Get(mVariableName), mIsFixed, mrModelPart.Nodes()); }

Perhaps you can help to shed some light here?

As discussed, to keep the functionality the same (and keep the initialize and finalize symmetric), we only touch the fixity of the nodes if mFixed is true (otherwise, we keep the fixity as is)

avdg81 · 2025-07-30T09:15:48Z

applications/GeoMechanicsApplication/custom_workflows/dgeoflow.cpp

+            Parameters{R"(
+            {
+                "variable_name"  : "WATER_PRESSURE",
+                "is_fixed"       : true
+            })"};


This unnamed object is destroyed immediately after creation. That's probably not what you intended.

Very good catch, thanks!

avdg81 · 2025-07-30T09:19:03Z

applications/GeoMechanicsApplication/custom_workflows/dgeoflow.cpp

+            Parameters{R"(
+            {
+                "variable_name"  : "WATER_PRESSURE",
+                "is_fixed"       : true


I'm not sure whether always setting this value to true is equivalent to what the code did previously. Do you know?

I think it's equivalent to setting the flag

avdg81 · 2025-07-30T09:21:25Z

applications/GeoMechanicsApplication/custom_workflows/dgeoflow.cpp

+    mProcesses.push_back(make_shared<GeoApplyConstantScalarValueProcess>(part, Parameters{R"(
+        {
+            "variable_name"  : "VOLUME_ACCELERATION_X",
+            "value"          : 0.0,
+            "is_fixed"       : true
+        })"}));
+
+    mProcesses.push_back(make_shared<GeoApplyConstantScalarValueProcess>(part, Parameters{R"(
+        {
+            "variable_name"  : "VOLUME_ACCELERATION_Y",
+            "value"          : -9.81,
+            "is_fixed"       : true
+        })"}));
+
+    mProcesses.push_back(make_shared<GeoApplyConstantScalarValueProcess>(part, Parameters{R"(
+        {
+            "variable_name"  : "VOLUME_ACCELERATION_Z",
+            "value"          : 0.0,
+            "is_fixed"       : true
+        })"}));


Agreed. In this way, we are also hard-coding the gravity direction and the value of the gravity acceleration (assuming all input is given in SI).

avdg81 · 2025-07-30T09:22:59Z

...pplication/tests/cpp_tests/custom_processes/test_geo_apply_constant_scalar_value_process.cpp

+                          KratosGeoMechanicsFastSuiteWithoutKernel)
+{
+    Model      current_model;
+    ModelPart& r_model_part = current_model.CreateModelPart("Main", 2);


Suggested change

ModelPart& r_model_part = current_model.CreateModelPart("Main", 2);

auto& r_model_part = current_model.CreateModelPart("Main", 2);

This suggestion also applies to the other new test cases.

avdg81 · 2025-07-30T09:37:16Z

...pplication/tests/cpp_tests/custom_processes/test_geo_apply_constant_scalar_value_process.cpp

+    const Geo::ConstVariableRefs nodal_variables = {};
+    ModelPart&                   r_model_part =
+        ModelSetupUtilities::CreateModelPartWithASingle2D3NElement(current_model, nodal_variables);


I'm just wondering whether this isn't more cumbersome than needed. Wouldn't it be sufficient to have an empty model part rather than a model part with nodes and an element?

This suggestion also applies to the next unit test.

You're right, simplified!

rfaasse

Processed the review comments

rfaasse · 2025-07-30T11:54:18Z

...cations/GeoMechanicsApplication/custom_processes/geo_apply_constant_scalar_value_process.cpp

-GeoApplyConstantScalarValueProcess::GeoApplyConstantScalarValueProcess(ModelPart& rModelPart, Parameters ThisParameters)
-    : Process(Flags()), mrModelPart(rModelPart)
+GeoApplyConstantScalarValueProcess::GeoApplyConstantScalarValueProcess(ModelPart& rModelPart,
+                                                                       const Parameters& ThisParameters)


rfaasse · 2025-07-30T11:55:06Z

...cations/GeoMechanicsApplication/custom_processes/geo_apply_constant_scalar_value_process.cpp


    mVariableName = ThisParameters["variable_name"].GetString();
-    this->Set(VARIABLE_IS_FIXED, ThisParameters["is_fixed"].GetBool());
+    mIsFixed      = ThisParameters.Has("is_fixed") ? ThisParameters["is_fixed"].GetBool() : false;


As discussed, it was handled by using the default parameters before, so the behavior hasn't changed. I implemented your suggestion, because indeed the default is already there in the header.

rfaasse · 2025-07-30T12:24:51Z

...cations/GeoMechanicsApplication/custom_processes/geo_apply_constant_scalar_value_process.cpp

+        << "' which is not of type Variable<double>.\n";
+
+    KRATOS_ERROR_IF_NOT(rModelPart.GetNodalSolutionStepVariablesList().Has(KratosComponents<VariableData>::Get(mVariableName)))
+        << "Trying to fix a variable that is not in the rModelPart - variable name is "


rfaasse · 2025-07-30T12:25:27Z

...cations/GeoMechanicsApplication/custom_processes/geo_apply_constant_scalar_value_process.cpp

+    if (mIsFixed && KratosComponents<Variable<double>>::Has(mVariableName)) {
+        VariableUtils().ApplyFixity(KratosComponents<Variable<double>>::Get(mVariableName), true,
+                                    mrModelPart.Nodes());
+    }


As discussed, to keep the functionality the same (and keep the initialize and finalize symmetric), we only touch the fixity of the nodes if mFixed is true (otherwise, we keep the fixity as is)

rfaasse · 2025-07-30T12:25:58Z

applications/GeoMechanicsApplication/custom_workflows/dgeoflow.cpp

+            Parameters{R"(
+            {
+                "variable_name"  : "WATER_PRESSURE",
+                "is_fixed"       : true
+            })"};


Very good catch, thanks!

rfaasse · 2025-07-30T12:26:13Z

applications/GeoMechanicsApplication/custom_workflows/dgeoflow.cpp

+            Parameters{R"(
+            {
+                "variable_name"  : "WATER_PRESSURE",
+                "is_fixed"       : true


I think it's equivalent to setting the flag

rfaasse · 2025-07-30T12:29:05Z

...pplication/tests/cpp_tests/custom_processes/test_geo_apply_constant_scalar_value_process.cpp

+                          KratosGeoMechanicsFastSuiteWithoutKernel)
+{
+    Model      current_model;
+    ModelPart& r_model_part = current_model.CreateModelPart("Main", 2);


rfaasse · 2025-07-30T12:32:07Z

...pplication/tests/cpp_tests/custom_processes/test_geo_apply_constant_scalar_value_process.cpp

+    const Geo::ConstVariableRefs nodal_variables = {};
+    ModelPart&                   r_model_part =
+        ModelSetupUtilities::CreateModelPartWithASingle2D3NElement(current_model, nodal_variables);


You're right, simplified!

avdg81

Thank you for processing the review suggestions. I feel this PR is ready to go.

rfaasse added 16 commits July 15, 2025 13:46

removed some clang-tidy issues

63bc3e3

Replace local flag with member

fe28a1b

Remove redundant get default parameters function

3c203dd

Removed redundant ';'

d2923f0

Fixed code

5f1080e

smell

Fixed explicit lambda capture

403eb1f

Minor changes and fixing an if-statement

9738ab4

Added some testing and clean-up

7979ba2

Removed redundant try catch blocks

f4e194c

Removed some redundant constructors

9f821e2

Removed some more redundant code/constructors

ee1b9e5

Simplification of the logic in constructor and in fixing the variables

ddf0f1f

Added some unit tests for the unhappy flows

f977e35

Removed redundant stream operator

98decf8

Added some includes (which were happening implicitly) and added some …

303f104

…forward declares

Added name to authors

68f596f

rfaasse requested a review from avdg81 July 25, 2025 13:37

rfaasse requested a review from a team as a code owner July 25, 2025 13:37

rfaasse commented Jul 25, 2025

View reviewed changes

Made sure the behavior stays exactly as it was

ba0d977

rfaasse self-assigned this Jul 29, 2025

avdg81 reviewed Jul 30, 2025

View reviewed changes

Processed review comments

41ac8ce

rfaasse commented Jul 30, 2025

View reviewed changes

rfaasse requested a review from avdg81 July 30, 2025 12:37

rfaasse enabled auto-merge (squash) July 30, 2025 12:37

avdg81 approved these changes Jul 30, 2025

View reviewed changes

rfaasse merged commit d6c901a into master Jul 30, 2025
10 checks passed

rfaasse deleted the geo/clean-up-process branch July 30, 2025 15:33

rfaasse linked an issue Jul 31, 2025 that may be closed by this pull request

[GeoMechanicsApplication] Degrees of freedom follow up issue: clean-up #13467

Closed

	const Parameters& ThisParameters)
	const Parameters& rParameters)

	mIsFixed = ThisParameters.Has("is_fixed") ? ThisParameters["is_fixed"].GetBool() : false;
	if (ThisParameters.Has("is_fixed")) mIsFixed = ThisParameters["is_fixed"].GetBool();

	ModelPart& r_model_part = current_model.CreateModelPart("Main", 2);
	auto& r_model_part = current_model.CreateModelPart("Main", 2);

Conversation

rfaasse commented Jul 25, 2025

Uh oh!

rfaasse Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avdg81 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rfaasse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avdg81 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rfaasse Jul 25, 2025 •

edited

Loading