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.

Sign up for GitHub

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

Jump to bottom

Geo/add time step executor #11649

Merged
merged 18 commits into from
Oct 5, 2023
Merged

Geo/add time step executor #11649

merged 18 commits into from
Oct 5, 2023

Conversation

WPK4FEM
Copy link
Contributor

@WPK4FEM WPK4FEM commented Oct 3, 2023

An executor to perform a single time step for GeoMechanical Analysis

It uses:

  • a solving strategy
  • a list of process references

and gives back

  • a time step end state containing the time, the convergence state and the number of iterations used.

7 unit tests for checking basic behavior are added.

@WPK4FEM WPK4FEM added the GeoMechanics Issues related to the GeoMechanicsApplication label Oct 3, 2023
@WPK4FEM WPK4FEM requested review from avdg81 and rfaasse October 3, 2023 08:44
rfaasse
rfaasse requested changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@rfaasse rfaasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very clear to me! I have some suggestions for improvements, but in general it looks good to me!

applications/GeoMechanicsApplication/custom_workflows/time_stepping.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_workflows/time_stepping.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_workflows/time_stepping.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_time_stepping.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_time_stepping.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_workflows/time_stepping.hpp Outdated Show resolved Hide resolved
@rfaasse rfaasse self-requested a review October 4, 2023 07:28
rfaasse
rfaasse previously approved these changes Oct 4, 2023
View reviewed changes
@rfaasse rfaasse self-requested a review October 4, 2023 07:28
@WPK4FEM WPK4FEM force-pushed the geo/add-time-step-executor branch from e4e2ecf to cb74f2c Compare October 4, 2023 07:55
avdg81
avdg81 reviewed Oct 4, 2023
View reviewed changes
Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really nice Wijtze Pieter! I have several minor remarks which may help to make the code even more elegant. See for yourself whether my comments make sense. Thanks!

applications/GeoMechanicsApplication/custom_workflows/strategy_wrapper.hpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/custom_workflows/time_step_executor.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_time_stepping.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_time_stepping.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_time_stepping.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_time_stepping.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_time_stepping.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_time_stepping.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_time_stepping.cpp Outdated Show resolved Hide resolved
applications/GeoMechanicsApplication/tests/cpp_tests/test_time_stepping.cpp Outdated Show resolved Hide resolved
avdg81 and others added 16 commits October 4, 2023 13:45
@avdg81 @WPK4FEM
Added a dummy class for executing a time step
1a778a4 
- For now, running a time step only returns a non-converged exit status.
- Added a unit test for this class.
@avdg81 @WPK4FEM
Added support for a solving strategy to the time step executer
6f8b4cc 
When the time step executer is requested to run, it returns whether the
solving strategy converged or not.
@avdg81 @WPK4FEM
Run now actually solves the problem by using the given strategy
0df0b22
@avdg81 @WPK4FEM
TimeStepExecuter now keeps a list of process references
85371f4 
When `Run()` is called the executer visits all processes and initializes
the solution step as well as finalizes it.
@avdg81 @WPK4FEM
A few minor changes
cc222a0 
- Extracted the solver's convergence state from the class template
  `TimeStepExecuter`, since there was one definition for each template
  instantiation. That was rather inconvenient.
- Added a new data structure that keeps the state at the end of a time
  step. It encompasses the convergence state as well as the number of
  nonlinear iterations.
@avdg81 @WPK4FEM
Renamed the test suite that contains the time executor tests
2bd2715
@avdg81 @WPK4FEM
Added time to the end state of a time step
13397b5 
In this way, we will know to which time step the other data applies.
@avdg81 @WPK4FEM
Renamed TimeStepExecuter to TimeStepExecutor
e775c9e
@WPK4FEM
unit tests for time step executor
b04e0ae
@WPK4FEM
Some tidying and adress sonar cloud reported code smells
8fdbf75
@WPK4FEM
Processed review comments from Richard Faasse
12f9e21
@WPK4FEM
Forgotten time_step_executor.cpp
235d644
@WPK4FEM
explicit to solve new reported code smell on constructor of DummyStra…
359e0f7 
…tegyWrapper.
@WPK4FEM
restored connection to time_step_end_state
8febf5d
@WPK4FEM
obsolete classes removed from test_time_stepping
9870064
@WPK4FEM
Review comments by Anne taken into account.
0fcb137
@WPK4FEM WPK4FEM force-pushed the geo/add-time-step-executor branch from cb74f2c to 0fcb137 Compare October 4, 2023 11:55
avdg81
avdg81 approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@avdg81 avdg81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one is ready to go.

@avdg81 avdg81 merged commit 970d762 into master Oct 5, 2023
@avdg81 avdg81 deleted the geo/add-time-step-executor branch October 5, 2023 08:28
@avdg81 avdg81 linked an issue Oct 5, 2023 that may be closed by this pull request
[GeoMechanicsApplication] Add a class for carrying out a single time step #11617
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avdg81 avdg81 avdg81 approved these changes

@rfaasse rfaasse Awaiting requested review from rfaasse

Assignees

@avdg81 avdg81

@WPK4FEM WPK4FEM

Labels
GeoMechanics Issues related to the GeoMechanicsApplication
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[GeoMechanicsApplication] Add a class for carrying out a single time step
3 participants
@WPK4FEM @avdg81 @rfaasse