[GeoMechanicsApplication] DSettlement non-convergence exit takes a lot longer than before #13176

markelov208 · 2025-02-28T10:02:16Z

📝 Description
A brief description of the PR.

added minimum allowable value for the time step to stop a computations when time step is too small
added unit tests for this change.

WPK4FEM · 2025-03-06T10:02:12Z

applications/GeoMechanicsApplication/custom_workflows/adaptive_time_incrementor.cpp

+            constexpr auto small_increment = 1.0e-3;
+            if (rResultantState.time + mDeltaTime > mEndTime - small_increment * mDeltaTime) {
+                mDeltaTime = mEndTime - rResultantState.time;
+            }


This condition is rewritten and does the same. It is now in a more logical place, executing the check less often.
However, the meaning of the variable small_increment has been changed from a time increment to a small factor.

Use the small_time_increment now to be consistent with python scrip

WPK4FEM · 2025-03-06T10:08:54Z

applications/GeoMechanicsApplication/python_scripts/geomechanics_analysis.py

+                KratosMultiphysics.Logger.PrintInfo(self._GetSimulationName(), "The time step ", self.delta_time, " is smaller than a given minimum value of ", self.min_delta_time)
+            else:
+                KratosMultiphysics.Logger.PrintInfo(self._GetSimulationName(), "The time step ", self.delta_time, " is smaller than a default minimum value of ", self.min_delta_time)
+            KratosMultiphysics.Logger.PrintInfo(self._GetSimulationName(), "Please check the case settings.")


What does case refer to here?

changed to "Please check settings in Project Parameters and Materials Files."

WPK4FEM · 2025-03-06T10:13:20Z

applications/GeoMechanicsApplication/python_scripts/geomechanics_analysis.py

@@ -102,6 +113,8 @@ def RunSolutionLoop(self):
                self.delta_time = new_time - t
                KratosMultiphysics.Logger.PrintInfo(self._GetSimulationName(), "Up-scaling to reach end_time without small increments: ", self.delta_time)

+            self._check_delta_time_size()


The lower limit for the delta_time is determined at line 106, line 113 could still slightly increase it. Therefore I understand that the check is here and not immediately after line 106.

WPK4FEM · 2025-03-06T10:17:41Z

applications/GeoMechanicsApplication/python_scripts/geomechanics_analysis.py

+        KratosMultiphysics.Logger.PrintInfo(self._GetSimulationName(), "STEP       : ", self._GetSolver().GetComputingModelPart().ProcessInfo[KratosMultiphysics.STEP])
+        KratosMultiphysics.Logger.PrintInfo(self._GetSimulationName(), "DELTA_TIME : ", self._GetSolver().GetComputingModelPart().ProcessInfo[KratosMultiphysics.DELTA_TIME])
+        KratosMultiphysics.Logger.PrintInfo(self._GetSimulationName(), "TIME       : ", self._GetSolver().GetComputingModelPart().ProcessInfo[KratosMultiphysics.TIME])


Why this change of layout, if its a real layout improved that increases readability of the log I'm o.k. with it, but generally we should be careful with changes like this. Users may have scripts that filter the log to retrieve stepping information. These scripts are likely to be dependent on the formatting.

I added more info for debugging then removed. Reverted changes now.

WPK4FEM · 2025-03-06T10:18:28Z

applications/GeoMechanicsApplication/tests/cpp_tests/custom_workflows/test_time_incrementor.cpp

@@ -9,6 +9,7 @@
 //
 //  Main authors:    Wijtze Pieter Kikstra
 //                   Anne van de Graaf
+//                   Richard Faasse


Why not Gennady too?

Because Richard gave more tests. I adapted them a little.

I think you still should add your own name here as well :)

WPK4FEM · 2025-03-06T10:28:07Z

applications/GeoMechanicsApplication/custom_workflows/adaptive_time_incrementor.cpp

+    KRATOS_ERROR_IF(mDeltaTime < mMinAllowableDeltaTime.second)
+        << "Delta time (" << mDeltaTime << ") is smaller than minimum allowable value "
+        << mMinAllowableDeltaTime.second << std::endl;


Like in the python route, I would expect the notice whether the minimum allowable value is the result of user input or a default. Now in adaptive_time_incrementor.h a std::pair is set up, but only its second value is ever used in a c++ piece of coding.

A very good catch

rfaasse

Clear improvement, I think this is almost ready to go, I just have a few minor suggestions/questions left

rfaasse · 2025-03-06T12:48:54Z

applications/GeoMechanicsApplication/custom_workflows/adaptive_time_incrementor.cpp

-        mDeltaTime *= mReductionFactor;
-    } else if (rResultantState.Converged() && (rResultantState.num_of_iterations < mMinNumOfIterations)) {
-        mDeltaTime = std::min(mDeltaTime * mIncreaseFactor, mMaxDeltaTime);
+    if (rResultantState.Converged()) // it is converged also at the beginning of the cycles


This comment is not completely clear to me yet, what do you mean with it?

rfaasse · 2025-03-06T12:49:56Z

applications/GeoMechanicsApplication/custom_workflows/adaptive_time_incrementor.cpp

+        if (rResultantState.time < mEndTime) {
+            // Up-scaling to reach end_time without small increments
+            constexpr auto small_increment_factor = 1.0e-3;
+            if (auto small_time_increment = small_increment_factor * mDeltaTime;


If I'm correct this could be const auto

Suggested change

if (auto small_time_increment = small_increment_factor * mDeltaTime;

if (const auto small_time_increment = small_increment_factor * mDeltaTime;

rfaasse · 2025-03-06T12:51:09Z

applications/GeoMechanicsApplication/custom_workflows/adaptive_time_incrementor.cpp

+        } else if (rResultantState.num_of_iterations == mMaxNumOfIterations) {
+            mDeltaTime *= mReductionFactor;
+        }


This seems quite a rare case to me: The calculation has converged exactly with the maximum number of iterations. Do we then want to reduce the time-step?

rfaasse · 2025-03-06T12:51:57Z

applications/GeoMechanicsApplication/custom_workflows/adaptive_time_incrementor.h

+    AdaptiveTimeIncrementor(double                         StartTime,
+                            double                         EndTime,
+                            double                         StartIncrement,
+                            std::pair<std::string, double> MinAllowableDeltaTime,


I think we also need to include , but probably better is to use a std::optional instead of a pair.

rfaasse · 2025-03-06T12:52:44Z

applications/GeoMechanicsApplication/tests/cpp_tests/custom_workflows/test_time_incrementor.cpp

@@ -9,6 +9,7 @@
 //
 //  Main authors:    Wijtze Pieter Kikstra
 //                   Anne van de Graaf
+//                   Richard Faasse


I think you still should add your own name here as well :)

avdg81

A very nice PR that makes the code more robust and consistent. Well done! I have a few suggestions that may help to simplify the code a bit more. I'll share a patch with you through Teams, just so you don't have to redo my suggestions.

avdg81 · 2025-03-06T10:21:36Z

applications/GeoMechanicsApplication/custom_workflows/adaptive_time_incrementor.h

-    std::size_t mMaxNumOfIterations;
+    double                         mEndTime;
+    double                         mDeltaTime;
+    std::pair<std::string, double> mMinAllowableDeltaTime;


I would suggest to change the type to std::optional<double>, to make clear that it may or may not have a user-defined value. Perhaps we should rename the data member to reflect that, e.g. mMaybeMinDeltaTime?

Please also note that so far, it doesn't seem like you were using the string value.

avdg81 · 2025-03-06T11:08:36Z

applications/GeoMechanicsApplication/custom_workflows/adaptive_time_incrementor.cpp

+AdaptiveTimeIncrementor::AdaptiveTimeIncrementor(double StartTime,
+                                                 double EndTime,
+                                                 double StartIncrement,
+                                                 std::pair<std::string, double> MinAllowableDeltaTime,


Let's move this new function parameter close to MaxTimeStepFactor, since the two parameters both bound the time increment.

avdg81 · 2025-03-06T12:38:43Z

applications/GeoMechanicsApplication/custom_workflows/adaptive_time_incrementor.cpp

+            constexpr auto small_increment_factor = 1.0e-3;
+            if (auto small_time_increment = small_increment_factor * mDeltaTime;
+                rResultantState.time + mDeltaTime > mEndTime - small_time_increment) {
+                mDeltaTime = mEndTime - rResultantState.time;
+            }


Let's avoid introducing a second minimum time increment. Let's use the minimum time increment that you'll introduce with this PR. For instance:

Suggested change

constexpr auto small_increment_factor = 1.0e-3;

if (auto small_time_increment = small_increment_factor * mDeltaTime;

rResultantState.time + mDeltaTime > mEndTime - small_time_increment) {

mDeltaTime = mEndTime - rResultantState.time;

}

if (rResultantState.time < mEndTime && mEndTime - rResultantState.time - mDeltaTime <

mMaybeMinDeltaTime.value_or(default_min_delta_time)) {

// Up-scaling to reach end_time without small increment

mDeltaTime = mEndTime - rResultantState.time;

}

Note that similar changes are required in the Python code.

avdg81 · 2025-03-06T13:02:54Z

applications/GeoMechanicsApplication/python_scripts/geomechanics_analysis.py

+        self.min_delta_time_set  = solver_settings["time_stepping"].Has("minimum_allowable_value")
+        self.min_delta_time      = solver_settings["time_stepping"]["minimum_allowable_value"].GetDouble() if self.min_delta_time_set else 1e-10


We can use a single attribute that is set to None when the user didn't provide a value. If we then make a getter for the minimum delta time, we can account for the default value when the value equals None.

markelov208 added 2 commits February 25, 2025 13:36

added unit test

3df6c93

added minimum allowable value for the time step

af922ed

markelov208 self-assigned this Feb 28, 2025

markelov208 added 10 commits February 28, 2025 15:12

fixed default minimum allowable time step

800d260

removed unused mTimeSpan

d3009b1

changed time step function

0e67804

fixed code smell

a176a87

removed unused variables

9bd1ee3

added unit test

9a936a0

replaced Exception with other exception types

0bca4e1

avoid small increment for non-finished computation

32f4198

fixed gcc compilation error

18e69e1

extended unit test for limit

449ac95

markelov208 requested review from avdg81, rfaasse and WPK4FEM March 5, 2025 17:15

WPK4FEM reviewed Mar 6, 2025

View reviewed changes

response to WPK comments

b6d6e6c

rfaasse reviewed Mar 6, 2025

View reviewed changes

avdg81 reviewed Mar 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GeoMechanicsApplication] DSettlement non-convergence exit takes a lot longer than before #13176

[GeoMechanicsApplication] DSettlement non-convergence exit takes a lot longer than before #13176

markelov208 commented Feb 28, 2025 •

edited

Loading

WPK4FEM Mar 6, 2025

markelov208 Mar 6, 2025

WPK4FEM Mar 6, 2025

markelov208 Mar 6, 2025

WPK4FEM Mar 6, 2025

WPK4FEM Mar 6, 2025

markelov208 Mar 6, 2025

WPK4FEM Mar 6, 2025

markelov208 Mar 6, 2025

rfaasse Mar 6, 2025

WPK4FEM Mar 6, 2025

markelov208 Mar 6, 2025

rfaasse left a comment

rfaasse Mar 6, 2025

rfaasse Mar 6, 2025

rfaasse Mar 6, 2025

rfaasse Mar 6, 2025

rfaasse Mar 6, 2025

avdg81 left a comment

avdg81 Mar 6, 2025

avdg81 Mar 6, 2025

avdg81 Mar 6, 2025

avdg81 Mar 6, 2025

	if (auto small_time_increment = small_increment_factor * mDeltaTime;
	if (const auto small_time_increment = small_increment_factor * mDeltaTime;

		self.min_delta_time_set = solver_settings["time_stepping"].Has("minimum_allowable_value")
		self.min_delta_time = solver_settings["time_stepping"]["minimum_allowable_value"].GetDouble() if self.min_delta_time_set else 1e-10

[GeoMechanicsApplication] DSettlement non-convergence exit takes a lot longer than before #13176

Are you sure you want to change the base?

[GeoMechanicsApplication] DSettlement non-convergence exit takes a lot longer than before #13176

Conversation

markelov208 commented Feb 28, 2025 • edited Loading

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

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

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

markelov208 commented Feb 28, 2025 •

edited

Loading