[GeoMechanicsApplication] Move normal load logic to cpp #11711

markelov208 · 2023-10-23T09:32:34Z

📝 Description

moved python class apply_normal_load_table_process to cpp

🆕 Changelog

created cpp class ApplyNormalLoadTableProcess
cleaned python script apply_normal_load_table_process .py
moved free functions from apply_scalar_constraint_table_process.cpp to a new class FunctionsForParameters
added ApplyNormalLoadTableProcess in dgeosettlement
added ApplyNormalLoadTableProcess in add_custom_process_to_python

- Corrected the return type of a copy assignment operator. - Added a missing default destructor for class `ApplyNormalLoadTableProcess`. - Added a missing `KRATOS_API(GEO_MECHANICS_APPLICATION)` (to export the class and make it available to the Python interface). - Added a missing pointer definition. - Removed an incorrect `return` statement from a Python script. - Fixed an incorrect condition (we missed the comparison to 0).

moved free functions from apply_scalar_constraint_table_process to a new class FunctionsForParameters added apply_normal_load_table_process in dgeosettlement

# Conflicts: # applications/GeoMechanicsApplication/custom_processes/apply_scalar_constraint_table_process.cpp

rfaasse

Nice clean and readable code! I only have 2 minor style related comments.

applications/GeoMechanicsApplication/custom_utilities/functions_for_parameters.cpp

applications/GeoMechanicsApplication/custom_utilities/functions_for_parameters.h

avdg81

Overall, your changes look nice and clean. I have several minor suggestions and a few things that need to be checked. Could you please take care of that? Thanks!

avdg81 · 2023-10-23T13:37:10Z

applications/GeoMechanicsApplication/custom_utilities/functions_for_parameters.h

To be consistent with the other files in the custom_utilities directory, I would suggest to rename this file to parameters_utilities.h. What do you think?

Fully agreed, so the files are renamed etc.

avdg81 · 2023-10-23T13:39:38Z

applications/GeoMechanicsApplication/custom_utilities/functions_for_parameters.h

+
+namespace Kratos {
+
+    class FunctonsForParameters


And perhaps rename the class to ParametersUtilities? Otherwise, I would at least correct a typo: FunctionsForParameters (note the missing i in Functons).

the class is renamed as yo usuggested.

avdg81 · 2023-10-23T13:53:09Z

applications/GeoMechanicsApplication/custom_processes/apply_normal_load_table_process.cpp

+{  
+    auto normal_parameters = FunctonsForParameters::ExtractParameters(rProcessSettings,
+                                                                      NamesOfSettingsToCopy);
+    normal_parameters.AddValue("value", rProcessSettings["value"][0]);


I was just wandering whether the index here (0) also refers to the normal component of value...? If yes, then we might consider to define that index in a wider scope (e.g. as a class constant) and then replace those indices by the new constant. You have done that already in member functions IsNormalComponentActive and IsTangentialComponentActive, but perhaps we can use it here as well. What do you think?

Indeed 0 and 1 are used in normal and tangential components. The constants are used now instead of the numbers.

avdg81 · 2023-10-23T13:57:02Z

applications/GeoMechanicsApplication/custom_processes/apply_normal_load_table_process.cpp

+                                                               "second_reference_coordinate",
+                                                               "specific_weight"});
+    if (FunctonsForParameters::HasTableAttached(rProcessSettings)) {
+        NamesOfSettingsToCopy.insert(NamesOfSettingsToCopy.end(), { "table" });


Although correct, I would simplify this to:

Suggested change

NamesOfSettingsToCopy.insert(NamesOfSettingsToCopy.end(), { "table" });

NamesOfSettingsToCopy.emplace_back("table");

Thank you. It is modified now.

avdg81 · 2023-10-23T14:02:52Z

applications/GeoMechanicsApplication/custom_processes/apply_normal_load_table_process.cpp

+                                                                "third_reference_coordinate",
+                                                                "specific_weight" });
+    if (FunctonsForParameters::HasTableAttached(rProcessSettings)) {
+        NamesOfSettingsToCopy.insert(NamesOfSettingsToCopy.end(), { "table" });


Suggested change

NamesOfSettingsToCopy.insert(NamesOfSettingsToCopy.end(), { "table" });

NamesOfSettingsToCopy.emplace_back("table");

avdg81 · 2023-10-23T14:13:34Z

applications/GeoMechanicsApplication/custom_processes/apply_normal_load_table_process.h

+//#include <memory>
+//#include <vector>


Although strictly speaking these #includes could be left out (they are indirectly included through processes/process.h), I would insist to actually include them, since the class declares a data member (mProcesses) that requires them. We don't want to rely on accidental includes from other includes.

avdg81 · 2023-10-23T14:16:43Z

applications/GeoMechanicsApplication/custom_processes/apply_normal_load_table_process.h

+
+    void ExecuteInitialize() override;
+    void ExecuteInitializeSolutionStep() override;
+


Recently, we have added an override for member function Info() to class ApplyScalarConstraintTableProcess, which comes in really handy during debugging. I would suggest you to add a similar override here too.

Thank you. Info() is added now.

avdg81 · 2023-10-23T14:31:03Z

applications/GeoMechanicsApplication/custom_utilities/functions_for_parameters.cpp

+        if (rSettings["table"][component].IsNumber()) {
+            return rSettings["table"][component].GetInt() != 0;
+        }
+
+        KRATOS_ERROR_IF_NOT(rSettings["table"][component].IsArray()) << "'table' is neither a single number nor an array of numbers";
+
+        const auto& table = rSettings["table"][component];
+        return std::any_of(table.begin(), table.end(),
+            [](const auto& value) {return value.GetInt() != 0; });
+    }


I doubt whether this is correct. My understanding is that table either has a single number or an array of numbers. Now this new overload checks one particular element from the array variant of table. By inspecting the original Python code, I don't have the impression that we might have a table which is an array, where each element contains an array in turn (which would require two indices to access a number). Therefore, I believe we can simplify this function body to:

Suggested change

if (rSettings["table"][component].IsNumber()) {

return rSettings["table"][component].GetInt() != 0;

}

KRATOS_ERROR_IF_NOT(rSettings["table"][component].IsArray()) << "'table' is neither a single number nor an array of numbers";

const auto& table = rSettings["table"][component];

return std::any_of(table.begin(), table.end(),

[](const auto& value) {return value.GetInt() != 0; });

}

return rSettings["table"][component].GetInt() != 0;

Could you please verify whether my understanding is correct? And if yes, please correct the implementation as suggested above. Thanks!

Thank you for pointing. I also found only [ "table"].GetInt() and ["table"][0/1/2].GetInt(). The check is removed now.

- removed unnecessary indentation - renamed functions_for_parameters to parameters_utilities - used normal/tangential component instead of 0/1 - used emplace_back("table") - reverted #include of <memory> and <vector> - added Info() - removed check if (rSettings["table"][component].IsNumber())

- removed KRATOS_ERROR_IF_NOT etc. for multi-component table

- Made a few member functions `const`. - Pass an object by reference to const rather than by value when it is not being modified.

rfaasse

Thanks for making the improvements, looks good!

rfaasse and others added 7 commits October 12, 2023 09:58

Added new class for the normal load

6c7b735

Removed an unused function and added an empty constructor

883381c

[Incomplete] Started to move logic from Python to C++

4f0a032

first version

8d884ff

introduced functions in apply_normal_load_table_process

7786dd0

moved free functions from apply_scalar_constraint_table_process to a new class FunctionsForParameters added apply_normal_load_table_process in dgeosettlement

replaced some of Parameters operations with vector<string>

507dc96

markelov208 requested review from avdg81 and rfaasse October 23, 2023 09:32

markelov208 self-assigned this Oct 23, 2023

Gennady Markelov added 2 commits October 23, 2023 13:41

Merge branch 'master' into geo/move-normal-load-logic-to-cpp

27eb803

# Conflicts: # applications/GeoMechanicsApplication/custom_processes/apply_scalar_constraint_table_process.cpp

resolved merge conflicts

0845900

rfaasse previously approved these changes Oct 23, 2023

View reviewed changes

applications/GeoMechanicsApplication/custom_utilities/functions_for_parameters.cpp Outdated Show resolved Hide resolved

applications/GeoMechanicsApplication/custom_utilities/functions_for_parameters.h Outdated Show resolved Hide resolved

avdg81 requested changes Oct 23, 2023

View reviewed changes

markelov208 dismissed rfaasse’s stale review via 40883ec October 24, 2023 08:33

Gennady Markelov and others added 2 commits October 24, 2023 11:55

- fixed indention

5820c8d

- removed KRATOS_ERROR_IF_NOT etc. for multi-component table

Fixed some issues reported by SonarQube

f315335

- Made a few member functions `const`. - Pass an object by reference to const rather than by value when it is not being modified.

avdg81 approved these changes Oct 24, 2023

View reviewed changes

rfaasse approved these changes Oct 24, 2023

View reviewed changes

avdg81 merged commit 374b7c3 into master Oct 24, 2023

avdg81 deleted the geo/move-normal-load-logic-to-cpp branch October 24, 2023 13:00

rfaasse mentioned this pull request Jan 24, 2025

[GeoMechanicsApplication] Migrating the ApplyNormalLoadTableProcess From Python to C++ #13036

Closed

rfaasse linked an issue Jan 24, 2025 that may be closed by this pull request

[GeoMechanicsApplication] Migrating the ApplyNormalLoadTableProcess From Python to C++ #13036

Closed

	NamesOfSettingsToCopy.insert(NamesOfSettingsToCopy.end(), { "table" });
	NamesOfSettingsToCopy.emplace_back("table");


		void ExecuteInitialize() override;
		void ExecuteInitializeSolutionStep() override;

		//#include <memory>
		//#include <vector>

[GeoMechanicsApplication] Move normal load logic to cpp #11711

[GeoMechanicsApplication] Move normal load logic to cpp #11711

Uh oh!

Conversation

markelov208 commented Oct 23, 2023

Uh oh!

rfaasse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

rfaasse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!