Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Iteration number based time stepping #2318

Merged
merged 26 commits into from Jan 22, 2019

Conversation

Projects
None yet
5 participants
@endJunction
Copy link
Member

endJunction commented Jan 14, 2019

The main algorithm was implemented quite some time already, but there was no driver for it. Here it goes.

I'd suggest commit-wise review.

EXECUTABLE_ARGS RichardsFlow_2d_small_iteration_adaptive_dt.prj
TESTER vtkdiff
REQUIREMENTS NOT OGS_USE_MPI
# No vtkdiff comparison here, because of the different file names for

This comment has been minimized.

@bilke

bilke Jan 14, 2019

Member

Then I guess you have to remove TESTER vtkdiff-line.

@wenqing
Copy link
Member

wenqing left a comment

Looks good!

dt = _min_ts;
else if (dt > _max_ts)
dt = _max_ts;
if (dt < _min_dt)

This comment has been minimized.

@wenqing

wenqing Jan 15, 2019

Member

dt = std::max(dt, _min_dt);

{
dt = _min_dt;
}
else if (dt > _max_dt)

This comment has been minimized.

@wenqing

wenqing Jan 15, 2019

Member

dt = std::min(dt, _max_dt);

_min_dt(min_dt),
_max_dt(max_dt),
_initial_dt(initial_dt),
_max_iter(_iter_times_vector.empty() ? 0 : _iter_times_vector.back())

This comment has been minimized.

@TomFischer

TomFischer Jan 15, 2019

Member

Sorry, I'm not so familiar with the time stepping. Could you give a simple and short example what data is stored in _iter_times_vector? Docu is below.

"\n\tSingleStep,"
"\n\tFixedTimeStepping,"
"\n\tEvolutionaryPIDcontroller,",
"\n\t IterationNumberBasedTimeStepping\n",

This comment has been minimized.

@TomFischer

TomFischer Jan 15, 2019

Member

Indentation of IterationNumberBasedTimeStepping is inconsistent with other time stepping schemas.

@TomFischer
Copy link
Member

TomFischer left a comment

Looks good.

@endJunction endJunction force-pushed the endJunction:TimeStepping branch 2 times, most recently from 446f83a to 961ca2a Jan 16, 2019

@bilke bilke force-pushed the ufz:master branch from 2a347a0 to a016b45 Jan 17, 2019

@endJunction endJunction force-pushed the endJunction:TimeStepping branch from 961ca2a to 744be99 Jan 17, 2019

@jbathmann

This comment has been minimized.

Copy link
Contributor

jbathmann commented Jan 17, 2019

I did some tests with the setup attached to this post on this and found some points, which were not exactly clear to me:

  • The algorithm seems to fail if nonlinear_solvers/nonlinear_solver/max_iter == max(number_iterations), because last element of list does not seem to be taken into account

  • The algorithm seems to fail, if max_iterations (same as above) > max(number_iterations), because multiplier is set to 1

  • I needed some time to figure out, what number_iterations and multiplier tag mean. Maybe some comments on this in the .md files would be really nice. Or did I miss on finding the documentation?

  • -The fixed output times from the project file are not hit. This might be the reason the lack of output files for the fixed times?

Hope this helps?
testsetup.zip

@endJunction endJunction force-pushed the endJunction:TimeStepping branch from 744be99 to 897c54e Jan 18, 2019

@endJunction endJunction force-pushed the endJunction:TimeStepping branch from 897c54e to 1e42372 Jan 18, 2019

endJunction added some commits Jan 10, 2019

[NL] Small refactorings, renames, braces, types.
Obligatory clang-format too.

Choosing int over size_t for number of iterations -- nobody
is going to wait that long anyway.

@endJunction endJunction force-pushed the endJunction:TimeStepping branch 2 times, most recently from edf5492 to 200c16f Jan 20, 2019

endJunction added some commits Jan 16, 2019

[NL] TimeStepping; Cleanup comments and docu.
Remove duplicate docu from the inherited functions.
[NL] INBTS; Special handling of first ts, first it
Otherwise the timestepper will repeat the second iteration
of the first time step with the initial_dt and simulation will
be aborted. In the current implementation the minimum dt
will be taken for the second iteration.
[NL] INBTS; Take multiplier for number of iters.
Change the way a multiplier is computed, s.t. for
a set of iterations and multipliers as:
i = 5 8 10
m = 2 1 0.5
following multiplier is taken
m(<5) = 2
m(5-7) = 2
m(8-9) = 1
m(>=10) = 0.5

This is easier to comprehend in the project files.

@endJunction endJunction force-pushed the endJunction:TimeStepping branch from 200c16f to edb12bd Jan 21, 2019

@endJunction

This comment has been minimized.

Copy link
Member Author

endJunction commented Jan 21, 2019

As @jbathmann mentioned above there are some issues with the repeating of the time steps for certain combinations of number_of_iterations vector and the max_iter in the non-linear solver; the simulations stops, although it could have succeeded. Increasing the max_iter seems to resolve this kind of problems.

Real fix need a major rewrite of the time loop.

Please review from the "[doc] Add IterationNumberBasedTS prj tags docu." commit 'til the end, especially the changes in the ProcessLib/UncoupledProcessesTimeLoop.cpp

@endJunction endJunction removed the WIP 🏗 label Jan 21, 2019

endJunction added some commits Jan 18, 2019

[PL] TS: Change handling when reaching end time.
First removal force the convergence checks to be executed.
Last two changes handle the t being (almost) equal to the
end time.

@endJunction endJunction force-pushed the endJunction:TimeStepping branch from edb12bd to d59bc2a Jan 21, 2019

@wenqing
Copy link
Member

wenqing left a comment

Looks good in these three commits.

@@ -431,7 +425,8 @@ double UncoupledProcessesTimeLoop::computeTimeStepping(
}
else
{
if (t < _end_time)
if (t < _end_time || std::abs(t - _end_time) <
std::numeric_limits<double>::epsilon())

This comment has been minimized.

@wenqing

wenqing Jan 21, 2019

Member

Just found a std function: std::islessequal.

This comment has been minimized.

@endJunction

endJunction Jan 21, 2019

Author Member

Thanks, I didn't know that function. But from cppreference it seems that the only difference to operator<=(x, y) is that the floating point exceptions are not raised (and we do like exceptions).

This comment has been minimized.

@wenqing

wenqing Jan 22, 2019

Member

I see.

@@ -451,7 +446,8 @@ double UncoupledProcessesTimeLoop::computeTimeStepping(
}
else
{
if (t < _end_time)
if (t < _end_time || std::abs(t - _end_time) <
std::numeric_limits<double>::epsilon())

This comment has been minimized.

@wenqing

wenqing Jan 21, 2019

Member

It can be replaced with std::islessequal.

@TomFischer
Copy link
Member

TomFischer left a comment

@endJunction endJunction merged commit 83cb86a into ufz:master Jan 22, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
deploy/netlify Deploy preview ready!
Details

@endJunction endJunction deleted the endJunction:TimeStepping branch Jan 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.