Add in riesz map and remove pc update#713
Conversation
| self.theta_solver_parameters = self._get_sub_params(opts, prefix + "theta_backsub") | ||
| self.exner_avg_solver_parameters = self._get_sub_params(opts, prefix + "exner_ave") | ||
| self.rho_avg_solver_parameters = self._get_sub_params(opts, prefix + "rho_ave") | ||
| self.riesz_map_parameters = self._get_sub_params(opts, prefix + "riesz_map") | ||
| self.scpc_solve_parameters = self._get_sub_params(opts, prefix + "scpc_solve") |
There was a problem hiding this comment.
This isn't necessary, just give each solver the right prefix and they will grab these options themselves.
There was a problem hiding this comment.
So something like:
self.exner_avg_solver = LinearVariationalSolver(
options_prefix=pc.getOptionsPrefix()+'exner_ave'
)
What about RieszMap which has no options_prefix as an argument?
There was a problem hiding this comment.
Arguably that is an oversight. Nevertheless, you can ask PETSc to get all the options with a particular prefix:
prefixed_options = PETSc.Options("prefix_") # This is another Options object
options_dict = PETSc.Options("prefix_").getAll() # this is a python dict for solver_parametersIf PETSc.Options() contains "prefix_obj_option" then PETSc.Options("prefix_") will contain "obj_option"`.
There was a problem hiding this comment.
But yes for the other solvers that is exactly what I meant.
| # bespoke hybridization preconditioner for this system owing to the | ||
| # nonlinear pressure gradient term. | ||
| r_tol = 1e-8 | ||
| a_tol = 1e-8 |
There was a problem hiding this comment.
The right atol depends on the dimension of the problem, so hardcoding it may lead to some unexpected behaviour if someone tries a problem with particularly different scales.
There was a problem hiding this comment.
You would suggest just not setting it at all?
There was a problem hiding this comment.
Personally no, but that's it is opinion, I was just pointing it out. If atol was part of the options before then it isn't up to me to decide gusto policy!
If the majority of use cases have scales O(earth) then it isn't unreasonable, it just needs bearing in mind.
There was a problem hiding this comment.
If both rtol and atol are set, does it stop as soon as either are met? I wonder which one we're hitting...
There was a problem hiding this comment.
Yes, it will stop as soon as any of atol, rtol, divtol, stol, or max_it are hit. If you are hitting divtol or stol you are definitely in trouble but it tends to be obvious.
The message from either ksp_converged_reason, ksp_converged_rate, or snes_converged_reason will say what convergence criteria was met.
atol might mean you stop prematurely if you have something with small physical scales that hasn't been nondimensionalised.
There was a problem hiding this comment.
@atb1995 @tommbendall do either of you have any logs that contain the convergence reasons? In general, I agree with Josh that we should not be setting atol but I am also wary of changing too much in one go!
There was a problem hiding this comment.
I imagine it is unlikely for you to be hitting atol for anything on the globe. But maybe for something local or nondimensionalised if the initial residual is O(1) or smaller.
There was a problem hiding this comment.
It is always hitting r_tol as far as I am aware, we are getting nowhere near 1e-8 atol for a lot of the runs I am doing at least
Co-authored-by: Josh Hope-Collins <joshua.hope-collins13@imperial.ac.uk>
Co-authored-by: Josh Hope-Collins <joshua.hope-collins13@imperial.ac.uk>
3 participants
Quick fixes to compressible solver to speed it up