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

Custom solver #734

Merged
merged 19 commits into from
Jun 5, 2024
Merged

Conversation

KulaginVladimir
Copy link
Collaborator

Proposed changes

Following #673, this PR introduces:

  • the possibility for the users to set the Newton solver preconditioner via F.Settings(preconditioner="...")
  • the possibilty for the users to set a custom Newton solver after the model (HeatTransferProblem) initialisation.

The expected usage of a custom Newton solver is:

my_model.initialise()

class CustomSolver(fenics.NewtonSolver):
     .....

custom_solver = CustomSolver()
custom_solver.parameters["..."] = ...

my_model.h_transport_problem.newton_solver = custom_solver

my_model.run()

The same can be done for for the F.ExtrinsicTrap class:

my_model.traps = [F.ExtrinsicTrap(...), ...]
for trap in my_model.traps:
    if isinstance(trap, F.ExtrinsicTrap)
        trap.newton_solver = CustomSolver()

and for the HeatTransferProblem (but before my_model.initialise() for the steady-state case!):

my_model.T = F.HeatTransferProblem(...)
my_model.T.newton_solver = CustomSolver()

Types of changes

What types of changes does your code introduce to FESTIM?

  • 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)
  • Code refactoring
  • Documentation Update (if none of the other choices apply)
  • New tests

Checklist

  • Black formatted
  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.53%. Comparing base (4573636) to head (b9fd3eb).
Report is 190 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #734      +/-   ##
==========================================
+ Coverage   99.52%   99.53%   +0.01%     
==========================================
  Files          58       59       +1     
  Lines        2501     2577      +76     
==========================================
+ Hits         2489     2565      +76     
  Misses         12       12              

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

@KulaginVladimir KulaginVladimir marked this pull request as ready for review April 3, 2024 00:46
@KulaginVladimir
Copy link
Collaborator Author

One thing to mention is that the info messages, shown when log_level=20, differ with f.NewtonSolver, since f.NonlinearVariationalSolver prints additional messages. A possible workaround is to invoke f.info before sovling the problem in FESTIM.

@RemDelaporteMathurin
Copy link
Collaborator

@KulaginVladimir thanks for this I think it is much needed to give more flexibility.

Would you mind listing the differences - if any - from the user point of view when running the default behaviour?
Are there any differences in absolute errors, usage, etc.

@RemDelaporteMathurin
Copy link
Collaborator

Also tagging @Allentro as he was interested in this feature

@RemDelaporteMathurin RemDelaporteMathurin added the enhancement New feature or request label Apr 3, 2024
@KulaginVladimir
Copy link
Collaborator Author

KulaginVladimir commented Apr 3, 2024

As I tested, there shouldn't appear significant differences (in errors or usage) with these changes if, for example, my_model.h_transport_problem.newton_solver is not provided manually.

During the model initialisation, a default fenics.NewtonSolver object is created and the solver parameters are set from my_model.settings: absolute_tolerance, relative_tolerance, maximum_iterations, linear_solver, and preconditioner, which were kept unchanged (the default preconditioner is set to default). Together with the introduced festim.Problem class, new changes should reproduce the solving procedure of f.NonlinearVariationalProblem and f.NonlinearVariationalSolver, used in the current version of FESTIM.

As I mentioned above, there is a change in fenics logs (i.e. when log_level=20). When NewtonSolver.solve is called, it doesn't print "Solving nonlinear variational problem.", whereas f.NonlinearVariationalSolver does.

@RemDelaporteMathurin is this what you asked? I also wonder about an example of using a custom solver. Shall we add it somewhere?

Comment on lines 124 to 128
f.begin(
"Solving nonlinear variational problem."
) # Add message to fenics logs
trap.newton_solver.solve(problem, trap.density[0].vector())
f.end()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, I added fenics.begin()/fenics.end() (see decription) to increase the indentation level of info messages printed by the Newton solver.

@RemDelaporteMathurin
Copy link
Collaborator

I would like to keep this open while we test it thoroughly. Playing with the solver can be a bit dangerous so I want to make sure we're not merging any breaking stuff.

@RemDelaporteMathurin RemDelaporteMathurin linked an issue Apr 5, 2024 that may be closed by this pull request
@@ -36,18 +38,33 @@ def __init__(
If None, the default fenics linear solver will be used ("umfpack").
More information can be found at: https://fenicsproject.org/pub/tutorial/html/._ftut1017.html.
Defaults to None.
preconditioner (str, optional): preconditioning method for the newton solver,
options can be veiwed by print(list_krylov_solver_preconditioners()).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
options can be veiwed by print(list_krylov_solver_preconditioners()).
options can be viewed by print(list_krylov_solver_preconditioners()).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Corrected

Copy link
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

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

Hi @KulaginVladimir my deepest apologies for taking so much time reviewing this.

I tested it and it seems to work perfectly well!

Several points:

  • If users specify a custom solver for say the H transport problem, then the parameters given in Settings (absolute, relative tolerances, etc.) will be ignored. This is ok, but we should add a print statement to inform the users
  • Can be done in a separate PR but it would be nice to add some documentation and maybe a short demo in the workshop?

Co-authored-by: Rémi Delaporte-Mathurin <[email protected]>
Copy link
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

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

Thank you so much for this awesome contribution @KulaginVladimir !!

@RemDelaporteMathurin RemDelaporteMathurin merged commit 8e0649d into festim-dev:main Jun 5, 2024
5 checks passed
@RemDelaporteMathurin RemDelaporteMathurin added this to the v1.3 milestone Jun 5, 2024
@KulaginVladimir KulaginVladimir deleted the CustomSolver branch July 26, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Solver
2 participants