feat: minimum interval for time-step increase after time-step cut #3338

victorapm · 2024-09-09T03:09:01Z

This PR addresses an issue where the simulation oscillates between time-step increases and cuts due to Newton loop stagnation.

For example, in one simulation, the time-step gradually increases up to a point leading to Newton loop stagnation. Then, the time-step was subsequently reduced, allowing the Newton loop to converge, but it quickly tried to revert back to the original large time-step, causing the same stagnation. This cycle would repeat for many cycles in the simulation.

This PR introduces a parameter to enforce a minimum interval before the time-step can increase again after a reduction occurs, thus preventing the repeated back-and-forth time-step adjustments and avoiding Newton iteration failures in both parts of the simulation.

Side question: we have places where we use "time step" and "time-step". Should we choose one format and use it consistently?

codecov · 2024-09-09T04:47:51Z

Codecov Report

Attention: Patch coverage is 70.27027% with 11 lines in your changes missing coverage. Please review.

Project coverage is 56.32%. Comparing base (5368ee3) to head (f2d5cac).
Report is 82 commits behind head on develop.

Files with missing lines	Patch %	Lines
...nents/physicsSolvers/NonlinearSolverParameters.cpp	55.55%	8 Missing ⚠️
...sSolvers/fluidFlow/CompositionalMultiphaseBase.cpp	40.00%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3338   +/-   ##
========================================
  Coverage    56.32%   56.32%           
========================================
  Files         1065     1065           
  Lines        90210    90230   +20     
========================================
+ Hits         50807    50823   +16     
- Misses       39403    39407    +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

paveltomin · 2024-09-09T15:50:42Z

src/coreComponents/physicsSolvers/SolverBase.cpp

@@ -401,14 +426,14 @@ real64 SolverBase::setNextDtBasedOnNewtonIter( real64 const & currentDt )
  if( newtonIter < iterIncreaseLimit )
  {
    // Easy convergence, let's increase the time-step.
-    nextDt = currentDt * m_nonlinearSolverParameters.timeStepIncreaseFactor();
+    nextDt = round( currentDt * m_nonlinearSolverParameters.timeStepIncreaseFactor() );


this is experimental, right?

Yes, it's not the main purpose of this PR. If the time-steps are larger than O(10) rounding shouldn't be an issue, right?

how about < 0.5 ? or i am missing something...

Yes, then this doesn't work

yeah, we should not use round here. For some cases we need very tiny time steps.

paveltomin · 2024-09-09T16:14:30Z

src/coreComponents/physicsSolvers/SolverBase.cpp

+   * */
+  if( dt < m_nextDt )
+  {
+    m_lastDtCut = -1;


so the logic only works with current "execute", no transfer to next cycle?

I did this because the current execute might decrease the time-step to meet a exact time target for the event. Then, suppose you come from a recent time-step cut, If I didn't reset m_lastDtCut, the code would not be able to increase the time step again (after having decreased not because of a noncoverged Newton, but because it had to meet a target time). Hope this explanation is not too confusing

it's ok, just might be confusing that logic is not applied across cycles
let's say we set minTimeStepIncreaseInterval=5 and it does a cut inside one cycle, then does let's say 2 succesful subtimesteps and goes into another cycle - then that 5-2 counter is lost?

Right, in this scenario, the cut information is lost for the next subEvent and I agree it shouldn't.

Maybe it's possible to improve the logic here. If you have some ideas, feel free to implement!

PS: in your scenario, if the time-step is not decreased to meet a exact target time, then the cut information is not lost in the next subEvent

CusiniM · 2024-09-09T18:11:58Z

This PR addresses an issue where the simulation oscillates between time-step increases and cuts due to Newton loop stagnation.

For example, in one simulation, the time-step gradually increases up to a point leading to Newton loop stagnation. Then, the time-step was subsequently reduced, allowing the Newton loop to converge, but it quickly tried to revert back to the original large time-step, causing the same stagnation. This cycle would repeat for many cycles in the simulation.

This PR introduces a parameter to enforce a minimum interval before the time-step can increase again after a reduction occurs, thus preventing the repeated back-and-forth time-step adjustments and avoiding Newton iteration failures in both parts of the simulation.

Side question: we have places where we use "time step" and "time-step". Should we choose one format and use it consistently?

I think that if we wanted to follow proper grammatical rules, it should be time step whenever it's a noun and time-step whenever it's adjectivized (e.g., time-step size, time-step control strategy).

castelletto1 · 2024-09-09T18:19:45Z

This PR addresses an issue where the simulation oscillates between time-step increases and cuts due to Newton loop stagnation.

For example, in one simulation, the time-step gradually increases up to a point leading to Newton loop stagnation. Then, the time-step was subsequently reduced, allowing the Newton loop to converge, but it quickly tried to revert back to the original large time-step, causing the same stagnation. This cycle would repeat for many cycles in the simulation.

This PR introduces a parameter to enforce a minimum interval before the time-step can increase again after a reduction occurs, thus preventing the repeated back-and-forth time-step adjustments and avoiding Newton iteration failures in both parts of the simulation.

Side question: we have places where we use "time step" and "time-step". Should we choose one format and use it consistently?

I think that if we wanted to follow proper grammatical rules, it should be time step whenever it's a noun and time-step whenever it's adjectivized (e.g., time-step size, time-step control strategy).

Why not simply timestep (Collins dictionary)? The timestep is set to ..., The timestep size ...

CusiniM · 2024-09-09T18:24:34Z

src/coreComponents/physicsSolvers/SolverBase.hpp

@@ -1008,6 +1011,9 @@ class SolverBase : public ExecutableGroup
  /// timestep of the next cycle
  real64 m_nextDt;

+  /// Number of cycles since last timestep cut
+  integer m_lastDtCut;


Suggested change

integer m_lastDtCut;

integer m_numTimestepsSinceLastDtCut;

CusiniM · 2024-09-09T18:31:07Z

This PR addresses an issue where the simulation oscillates between time-step increases and cuts due to Newton loop stagnation.
For example, in one simulation, the time-step gradually increases up to a point leading to Newton loop stagnation. Then, the time-step was subsequently reduced, allowing the Newton loop to converge, but it quickly tried to revert back to the original large time-step, causing the same stagnation. This cycle would repeat for many cycles in the simulation.
This PR introduces a parameter to enforce a minimum interval before the time-step can increase again after a reduction occurs, thus preventing the repeated back-and-forth time-step adjustments and avoiding Newton iteration failures in both parts of the simulation.
Side question: we have places where we use "time step" and "time-step". Should we choose one format and use it consistently?

I think that if we wanted to follow proper grammatical rules, it should be time step whenever it's a noun and time-step whenever it's adjectivized (e.g., time-step size, time-step control strategy).

Why not simply timestep (Collins dictionary)? The timestep is set to ..., The timestep size ...

The Oxford dictionary does not have the entry as a single word but it would really simplify our life. Spellcheckers don't recognize it though.

CusiniM · 2024-09-09T18:36:32Z

Talking about the actual goal of the PR... The logic is a bit clunky but we certainly need to include some more info about past convergence when selecting the new dt so this could be a start.

I don't think we can have all those round calls though coz we have often test cases (for like lab scale models) in which we use dt~ 1e-3 s so that would mess up things. @victorapm if you were tyring to round up the values to improve the readability of the output we can probably do it using the units utilities

GEOS/src/coreComponents/common/Units.hpp

Line 215 in 14a21b5

struct TimeFormatInfo

victorapm · 2024-09-09T18:58:01Z

Updated the variable name and removed round in 2302936

Let me know what you guys think!

I suggest voting for deciding wheter to use timestep, time-step, ...

paveltomin · 2024-09-18T17:24:35Z

please also merge https://github.com/GEOS-DEV/SPE11/tree/paludettomag1/spe11c once this PR is done

victorapm · 2024-09-18T17:29:50Z

It seems we'll need to rebaseline given the new logic and default value for m_numTimestepsSinceLastDtCut, which I think is good to have. @CusiniM

rrsettgast

When increasing tilmestep, wouldn't it be appropriate to modify the time-step increase size based on the number of newton iterations the current step took to converge? So instead of: "if numNewtonIter < 8 then nextDt = dt * factor" we have the factor be dependent on numNewtonIter?

rrsettgast · 2024-09-18T17:48:29Z

src/coreComponents/physicsSolvers/NonlinearSolverParameters.cpp

+  registerWrapper( viewKeysStruct::minTimeStepIncreaseIntervalString(), &m_minTimeStepIncreaseInterval ).
+    setApplyDefaultValue( 10 ).
+    setInputFlag( InputFlags::OPTIONAL ).
+    setDescription( "Value of the minimum interval, since the last time-step cut, for increasing the time-step." );


what does "minimum interval" mean? It is hard to tell from this description. is it "minimum number of time steps"?

Right, it's minimum number of time steps. Let me update the comment

I find the comment below very clear:

GEOS_LOG_LEVEL_RANK_0( 1, GEOS_FMT( "{}: time-step size will be kept the same since it's been {} cycles since last cut.", getName(), m_numTimestepsSinceLastDtCut ) );

What about renaming m_minNumCyclesFixedTimeStepAfterCut. It's a bit wordy but a probably clearer?

Better m_minFixedTimeStepNumCyclesAfterCut

I am fine with all options presented

rrsettgast · 2024-09-18T17:55:20Z

src/coreComponents/physicsSolvers/NonlinearSolverParameters.cpp


  registerWrapper( viewKeysStruct::timeStepDecreaseFactorString(), &m_timeStepDecreaseFactor ).
    setApplyDefaultValue( 0.5 ).
    setInputFlag( InputFlags::OPTIONAL ).
-    setDescription( "Factor by which the time step is decreased when the number of Newton iterations is large." );
+    setDescription( "Factor by which the time-step is decreased when the number of Newton iterations is large." );

  registerWrapper( viewKeysStruct::timeStepIncreaseFactorString(), &m_timeStepIncreaseFactor ).
    setApplyDefaultValue( 2.0 ).


I don't think this default should be 2.0....should it?

I agree that the default could be a little more conservative.

That makes sense, but this is not the variable I'm adding in this PR (m_timeStepIncreaseFactor). Should we make the change here or in another PR?

Yes, not in this PR

src/coreComponents/physicsSolvers/SolverBase.cpp

castelletto1

Except for the suggestion on rewording (optional), looks good to me. Thank you!

Co-authored-by: Nicola Castelletto <38361926+castelletto1@users.noreply.github.com>

castelletto1 · 2024-09-18T19:15:42Z

I suggest voting for deciding whether to use timestep, time-step, ...

Let's postpone the renaming to a future PR and consistently apply the change everywhere in GEOS.

With all due respect to the Oxford Dictionary, my preference is to use timestep.

…xclusions

victorapm added 2 commits September 8, 2024 19:36

Fix build without SuperLU-Dist support

86e7e44

Add minDtIncreaseInterval and update time-stepping logic

e2de6f0

victorapm requested review from CusiniM, paveltomin, klevzoff, francoishamon, rrsettgast and castelletto1 as code owners September 9, 2024 03:09

victorapm self-assigned this Sep 9, 2024

victorapm added the flag: ready for review label Sep 9, 2024

victorapm changed the title ~~feature: minimum interval for time-step increase after time-step cut~~ feat: minimum interval for time-step increase after time-step cut Sep 9, 2024

Fix code style

0eea818

paveltomin reviewed Sep 9, 2024

View reviewed changes

CusiniM reviewed Sep 9, 2024

View reviewed changes

Addressing review

2302936

Minor fix

64c6250

paveltomin approved these changes Sep 9, 2024

View reviewed changes

Merge branch 'develop' into feature/paludettomag1/timestep

cbadc6d

CusiniM approved these changes Sep 12, 2024

View reviewed changes

victorapm added ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI labels Sep 12, 2024

victorapm added 2 commits September 12, 2024 14:35

Merge branch 'develop' into feature/paludettomag1/timestep

edc136b

Merge branch 'develop' into feature/paludettomag1/timestep

dae369c

rrsettgast approved these changes Sep 18, 2024

View reviewed changes

rrsettgast added the flag: requires rebaseline Requires rebaseline branch in integratedTests label Sep 18, 2024

castelletto1 reviewed Sep 18, 2024

View reviewed changes

src/coreComponents/physicsSolvers/SolverBase.cpp Outdated Show resolved Hide resolved

castelletto1 approved these changes Sep 18, 2024

View reviewed changes

Use getter for minTimeStepIncreaseInterval

e5f3df7

Co-authored-by: Nicola Castelletto <38361926+castelletto1@users.noreply.github.com>

victorapm and others added 4 commits September 18, 2024 15:17

Update comment about viewKeysStruct::minTimeStepIncreaseIntervalString

9e34580

Merge branch 'develop' into feature/paludettomag1/timestep

a20c4a0

modify the processIntegratedTestCheckFailures.py to have no default e…

37ff758

…xclusions

Update baselines

b6e196a

victorapm requested review from cssherman, wrtobin and jhuang2601 as code owners September 20, 2024 00:14

victorapm and others added 4 commits September 19, 2024 20:14

Merge branch 'develop' into feature/paludettomag1/timestep

dff5aba

Minor

985f8cc

change ats config for c1_ppy.ats...they appear invalid

9f62c4a

rebaseline

f2d5cac

rrsettgast merged commit 1616ea4 into develop Sep 20, 2024
25 checks passed

rrsettgast deleted the feature/paludettomag1/timestep branch September 20, 2024 06:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: minimum interval for time-step increase after time-step cut #3338

feat: minimum interval for time-step increase after time-step cut #3338

victorapm commented Sep 9, 2024

codecov bot commented Sep 9, 2024 •

edited

Loading

paveltomin Sep 9, 2024

victorapm Sep 9, 2024

paveltomin Sep 9, 2024

victorapm Sep 9, 2024

CusiniM Sep 9, 2024

paveltomin Sep 9, 2024

victorapm Sep 9, 2024

paveltomin Sep 9, 2024

victorapm Sep 9, 2024

victorapm Sep 9, 2024

CusiniM commented Sep 9, 2024

castelletto1 commented Sep 9, 2024

CusiniM Sep 9, 2024

CusiniM commented Sep 9, 2024

CusiniM commented Sep 9, 2024

victorapm commented Sep 9, 2024

paveltomin commented Sep 18, 2024

victorapm commented Sep 18, 2024

rrsettgast left a comment

rrsettgast Sep 18, 2024

victorapm Sep 18, 2024

castelletto1 Sep 18, 2024 •

edited

Loading

castelletto1 Sep 18, 2024

victorapm Sep 18, 2024

rrsettgast Sep 18, 2024

castelletto1 Sep 18, 2024

victorapm Sep 18, 2024

castelletto1 Sep 18, 2024

castelletto1 left a comment •

edited

Loading

castelletto1 commented Sep 18, 2024

feat: minimum interval for time-step increase after time-step cut #3338

feat: minimum interval for time-step increase after time-step cut #3338

Conversation

victorapm commented Sep 9, 2024

codecov bot commented Sep 9, 2024 • edited Loading

Codecov Report

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

CusiniM commented Sep 9, 2024

castelletto1 commented Sep 9, 2024

Choose a reason for hiding this comment

CusiniM commented Sep 9, 2024

CusiniM commented Sep 9, 2024

victorapm commented Sep 9, 2024

paveltomin commented Sep 18, 2024

victorapm commented Sep 18, 2024

rrsettgast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

castelletto1 Sep 18, 2024 • 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

castelletto1 left a comment • edited Loading

Choose a reason for hiding this comment

castelletto1 commented Sep 18, 2024

codecov bot commented Sep 9, 2024 •

edited

Loading

castelletto1 Sep 18, 2024 •

edited

Loading

castelletto1 left a comment •

edited

Loading