-
Notifications
You must be signed in to change notification settings - Fork 11
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
Solver Convergence Improvements #536
Conversation
gusto/solvers/linear_solvers.py
Outdated
@@ -33,13 +33,19 @@ | |||
class TimesteppingSolver(object, metaclass=ABCMeta): | |||
"""Base class for timestepping linear solvers for Gusto.""" | |||
|
|||
def __init__(self, equations, alpha=0.5, solver_parameters=None, | |||
overwrite_solver_parameters=False): | |||
def __init__(self, equations, alpha=0.5, tau_u=None, tau_r=None, tau_t=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be a dictionary of tau
values, where the keys are variable names? Otherwise this is specific to the compressible Euler equations whereas it would ideally be agnostic to equation type...
The other alternative is to not include this as an argument here.
(The complication being that this only works for shallow-water currently but that's a separate thing!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've switched to a dictionary, with defaulting to alpha if the dictionary has no entry for a given field
gusto/solvers/linear_solvers.py
Outdated
quadrature_degree=None, solver_parameters=None, | ||
overwrite_solver_parameters=False): | ||
""" | ||
Args: | ||
equations (:class:`PrognosticEquation`): the model's equation. | ||
alpha (float, optional): the semi-implicit off-centring factor. | ||
Defaults to 0.5. A value of 1 is fully-implicit. | ||
tau_u (float, optional): the semi-implicit relaxation parameter for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a problem with each of the tau values being a separate argument here, but you could also make this a dictionary if you decide to go down that route.
gusto/solvers/linear_solvers.py
Outdated
@@ -52,6 +58,23 @@ def __init__(self, equations, alpha=0.5, solver_parameters=None, | |||
self.dt = equations.domain.dt | |||
self.alpha = alpha | |||
|
|||
# Set relaxation parameters. If an alternative has not been given, set | |||
# to semi-implicit off-centering factor | |||
if tau_u is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these could actually be fit on one line nicely, e.g.
self.tau_u = tau_u if tau_u is not None else alpha
""" | ||
for field_name in transported_field_names: | ||
if field_name != 'u': | ||
logger.info(f'SIQN: Zeroing Implicit forcing for {field_name}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the SIQN
in the logged message now needs to be Semi-Implicit Quasi Newton
…ject/gusto into solver_improvements
Co-authored-by: Dr Jemma Shipton <j.shipton@exeter.ac.uk>
Co-authored-by: Dr Jemma Shipton <j.shipton@exeter.ac.uk>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Alex!
Importing solver improvements we have in GungHo:
tau values for linear solver which accelerate convergence if set to 1. This has been shown to make the moist baroclinic in channel stable for 2x2 SIQN with tau_t and tau_r = 1. Description of why these work has been attached in
Tau_Values.pdf
Accelerate solver convergence by zeroing implicit forcing's for transport terms
These changes have been added on switches, so do not change KGOs.