Skip to content

Conversation

@mariusnevland
Copy link
Contributor

@mariusnevland mariusnevland commented Jun 12, 2025

Proposed changes

Regarding issue #1441. This PR introduces the option to filter residual and increment norms by equations and variables, respectively, to be used in the convergence checks. This PR introduces a separate mixin allowing to switch between the standard convergence check and a multiphysics one.

Minor breaking change: reference_residual is removed from the signature of the check_convergence method. Upstream adaptations are made to cleanup.

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.

@jwboth
Copy link
Contributor

jwboth commented Jun 19, 2025

Next steps:

  • Extend one multiphysics tutorial (e.g. poromechanics) and add a section on multiphysics convergence checks/norms. Showcase how to setup the model and run with mulitphysics norms by using mixins and assigning tolerances. This will help both to provide a structure for how to extend some tests in the suite, as well as provide a documentation/introduction.
  • Extend the docstrings of check_convergence and provide a description of the relative norm concept and how it manages the convergence check, including hints what keywords to use in the parameter dictionary.
  • Remove the use of a reference residual in old and new check_convergence (and upstream). The choice of the relative residual etc. is not taken care of by check_convergence.
  • Apply multiphysics norms to an existing test. I suggest test_pull_south_positive_reference_pressure as it aims at non-trivial solution state.
  • Fix typing and mypy issues.
  • Synchronize with latest addition of divergence criterion.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jwboth jwboth marked this pull request as ready for review June 26, 2025 19:07
@jwboth
Copy link
Contributor

jwboth commented Dec 18, 2025

Most important changes which require a critical look:

  • No changes have been made to __init__. Should all objects be included that is referred to from files like solvers, statistics, parameters etc?
  • The user interface I: The keywords to control the default convergence criteria - see nonlinear_solver.py. The default user will only be exposed to these. Examples: nl_max_iterations, nl_convergence_inc_atol, nl_convergence_res_rtol, divergence_inc_atol etc.
  • The user interface II: The class names for the single convergence criteria. Do you have better suggestions than quite long names? Only those users will be exposed to these who want to build their custom problem specific stopping criteria which are not covered by the default criteria. Examples: IncrementBasedAbsoluteCriterion, `EquationBased
  • The interna: The class names used across solvers, statistics, run_models like SimulationStatus, ConvergenceStatus, ConvergenceStatusSummary, ConvergenceStatusCollection. Are they OK? In particular the last three. All class names are not supposed to be used by users such that this should be easy to change in the future.
  • Extension of specific volumes to boundary grids
  • Introduction of pp.SimulationStatus as main status object for time step control. I am happy to unroll the handling of time stepping and revise this part. As of now, it is more a necessesity to allow for communication between diverging solvers and time step discretization in run_models. As now lists of convergence criteria is allowed, a "scalar" object is used for passing the overall status.
  • The logic behind defining convergence criteria (good places for this are the convergence_criterion.ipynb and the definition of the default convergence criteria as part of the pp.NewtonSolver)
  • The use of convergence criteria in pp.LinearSolver. The motivation here was to be close to pp.NewtonSolver but maybe this is overkill? I do not understand why mypy is strict here, and does not allow for the same initialization of the convergence and divergence criteria. If you have a better idea than surpressing mypy, please help.

else:
if self.reference_value is not None:
return
self.reference_value = reference_value
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check that reference_value is not 0.0, as in the dict case?

# subdomain neighbor.
assert all(isinstance(g, pp.BoundaryGrid) for g in grids), "Mixed grids"

# Return 1's for 0D boundaries
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be all boundaries?

@keileg
Copy link
Contributor

keileg commented Dec 19, 2025

No changes have been made to init. Should all objects be included that is referred to from files like solvers, statistics, parameters etc?

I would say that files that are (somewhat frequently) accessed by the user should go in init, else direct imports are okay.

The user interface I: The keywords to control the default convergence criteria - see nonlinear_solver.py. The default user will only be exposed to these. Examples: nl_max_iterations, nl_convergence_inc_atol, nl_convergence_res_rtol, divergence_inc_atol etc.

As mentioned yesterday, I think this is fine. @IvarStefansson, do you agree?

The user interface II: The class names for the single convergence criteria. Do you have better suggestions than quite long names? Only those users will be exposed to these who want to build their custom problem specific stopping criteria which are not covered by the default criteria. Examples: IncrementBasedAbsoluteCriterion, `EquationBased

As a rule, we have preferred explicit names. Having looked at the specifics, I have no objections. I will add some documentation to these classes, though.

The interna: The class names used across solvers, statistics, run_models like SimulationStatus, ConvergenceStatus, ConvergenceStatusSummary, ConvergenceStatusCollection. Are they OK? In particular the last three. All class names are not supposed to be used by users such that this should be easy to change in the future.

I they are fine.

Extension of specific volumes to boundary grids

I see no problems. @IvarStefansson, I suppose you have had a look at this? Just to be sure.

Introduction of pp.SimulationStatus as main status object for time step control. I am happy to unroll the handling of time stepping and revise this part. As of now, it is more a necessesity to allow for communication between diverging solvers and time step discretization in run_models. As now lists of convergence criteria is allowed, a "scalar" object is used for passing the overall status.

I have no objections here. I realize we may need to further improve the flow of information when attacking issues related to the time stepping, but this to me looks like a step in the right direction.

The logic behind defining convergence criteria (good places for this are the convergence_criterion.ipynb and the definition of the default convergence criteria as part of the pp.NewtonSolver)

I think these are reasonable. There are other ways of doing this and I suspect we will change things in the future, e.g., change the default criteria, but let's leave the code as it is for now.

The use of convergence criteria in pp.LinearSolver. The motivation here was to be close to pp.NewtonSolver but maybe this is overkill? I do not understand why mypy is strict here, and does not allow for the same initialization of the convergence and divergence criteria. If you have a better idea than surpressing mypy, please help.

I agree it is reasonable to aim for unity. My preferred design (which goes way beyond the scope of this project) is to do away with the notion of a linear problem altogether and let Newton sort out everything, but I'm fine with the current compromise.

# subdomain neighbor.
assert all(isinstance(g, pp.BoundaryGrid) for g in grids), "Mixed grids"

# Return 1's for 0D boundaries
Copy link
Contributor

Choose a reason for hiding this comment

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

While I remember it, we should make sure this is tested. This entails having a grid with a fracture that extends to the boundary, as the specific volume of the highest dimensional grid is 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants