Trial time

[IvarStefansson]

Proposed changes

Contributions to PorePy are highly appreciated. Clearly explain why this pull request (PR) is needed and why it should be accepted. If this PR solves an issue, explain how it is done. Please, also summarise the changes to the code.

Types of changes

What types of changes does this PR introduce to PorePy?
Put an x in the boxes that apply.

• Minor change (e.g., dependency bumps, broken links).
• Bugfix (non-breaking change which fixes an issue).
• New feature (non-breaking change which adds functionality).
• Breaking change (fix or feature that would cause existing functionality to not work as expected).
• Testing (contribution related to testing of existing or new functionality).
• Documentation (contribution related to adding, improving, or fixing documentation).
• Maintenance (e.g., improve logic and performance, remove obsolete code).
• Other:

Checklist

Put an x in the boxes that apply or explain briefly why the box is not relevant.

• The documentation is up-to-date.
• Static typing is included in the update.
• This PR does not duplicate existing functionality.
• The update is covered by the test suite (including tests added in the PR).
• If new skipped tests have been introduced in this PR, pytest was run with the --run-skipped flag.

[Yuriyzabegaev]

I've ran through the changes and can say, it's a solid step in the right direction! Some thoughts here.

I like how we distinguish between statuses ACCEPTED, REJECTED, and STOPPED, but I feel like we need to document what the latter is used for. REJECTED applies to both cases of divergence and iteration limit. Was the intention of STOPPED to indicate a critical failure, when we should not attempt to decrease the time step and retry? E.g., when both state and iterate arrays somehow became nans. Do we even need to consider this situation?

The old TimeManager now has 2 responsibilities: (i) to be a configuration data-structure, (ii) to compute the new time step based on feedback. Could we go even further and decouple it into 2 objects? Also, it's a part of the model, but it's probably natural to move it to be a part of the time stepper?.

The time stepper class is composed of 3 components. TimeStepAcceptanceCriteria, TimeStepRejectionCriteria and TimeManager (logically). What I think would be very nice to have is the ability to customize these components individually via dependency injection, i.e., the user passes their own TimeStepAcceptanceCriteria in the model runner parameters, and they are passed to TimeManager.__init__. This would make it flexible and testable. The same applies to the TimeStepper itself: make the TimeStepperFactory provide a custom instance of a TimeStepper, if the user passes it.

So to say, I really like where this project is going, good job @jwboth! Will add a few minor notes in the code below.

[jwboth]

Thanks @Yuriyzabegaev for having a look. I was not aware of the fact that this code snippet was ready :) The idea here was just to provide the orchestration skeleton underlining the integration of the time manager.

I like how we distinguish between statuses ACCEPTED, REJECTED, and STOPPED, but I feel like we need to document what the latter is used for. REJECTED applies to both cases of divergence and iteration limit. Was the intention of STOPPED to indicate a critical failure, when we should not attempt to decrease the time step and retry? E.g., when both state and iterate arrays somehow became nans. Do we even need to consider this situation?

STOPPED indeed associates with critical failure. The idea has been to use the same style as in NewtonSolver. It will be possible to distinguish which criterion failed from an info object. But I may want to discuss it, as the current solution may be too verbose (it stores essentially everything).

The old TimeManager now has 2 responsibilities: (i) to be a configuration data-structure, (ii) to compute the new time step based on feedback. Could we go even further and decouple it into 2 objects? Also, it's a part of the model, but it's probably natural to move it to be a part of the time stepper?.

I all pro chopping it up into smaller pieces. Before doing that, one has to decide how time steps are cut. Should there be always 1 rule or allow we for multiple rules? Or will this be the responsibility/design choice of a class that shall suggest time step sizes? The model does only need to know time and dt, nothing more.

The time stepper class is composed of 3 components. TimeStepAcceptanceCriteria, TimeStepRejectionCriteria and TimeManager (logically). What I think would be very nice to have is the ability to customize these components individually via dependency injection, i.e., the user passes their own TimeStepAcceptanceCriteria in the model runner parameters, and they are passed to TimeManager.init. This would make it flexible and testable. The same applies to the TimeStepper itself: make the TimeStepperFactory provide a custom instance of a TimeStepper, if the user passes it.

Fully agreed. We can talk about it. Current idea was to end up with a similar design as for NewtonSolver, where it is also possible to customize criteria from solver_params.

I will continue once #1684 is merged as it also touched the model runner - the main idea was actually to throw away this branch... but now its here.

[IvarStefansson]

I like the design. I think we should leave what is left in the TimeManager for this PR as far as possible.

Let's meet and discuss how to proceed, @Yuriyzabegaev.

[Yuriyzabegaev]

I highlighted some important changes, hope it helps reviewing it.

[keileg]

Partial review, I ran out of energy before having looked at the tests in earnest. I will try to return to these later today.