-
Notifications
You must be signed in to change notification settings - Fork 13
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
Terminator toy #457
Terminator toy #457
Conversation
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.
This looks really good Tim, thanks! Just three really minor comments
pairing physics parametrisations and timestepping schemes to use | ||
for each. Timestepping schemes for physics can be explicit | ||
or implicit. Defaults to None. | ||
prescribed_transporting_velocity: (field, optional): A known |
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 you add a bit more description to this to explain that it can be a python function, taking time as an argument?
I see that the docstrings are wrong for the PrescribedTransport
stepper, sorry about that!
dt (:class:`Constant`): the time interval for the scheme. | ||
""" | ||
|
||
logger.info(f'Evaluating physics parametrisation {self.label.label}') |
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.
Maybe add a pass
underneath this to make it obvious that this evaluate method does nothing
gusto/physics.py
Outdated
label_name = 'terminator_toy' | ||
super().__init__(equation, label_name, parameters=None) | ||
|
||
assert species1_name in equation.field_names, f"Field {species1_name} does not exist in the equation set" |
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.
Very picky point here. It might be slightly better if we change this error (and the one below) to something like:
if species1_name not in equation.field_names:
raise ValueError(...)
as this is the normal type of python error when the wrong argument is given to a routine
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.
Cool, I've done that for the two name checks. Is the check for the function spaces ok as it is, as this is not an argument error, but a compatibility error?
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 check is OK as is, as you say it is not about having an incorrect argument but that the model set up isn't correct
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 very much Tim! We've come up with a plan of follow-up activities for this to get the new things tested, that I'll post here for posterity:
- a test for the
TerminatorToy
- add
SplitPrescribedTransport
as an parametrized option intest_prescribed_transport.py
- use
SplitPrescribedTransport
in one of the physics tests, maybetest_precipitation.py
This adds the physics scheme for the Terminator Toy problem, which tests the interaction of two species, defined through mixing ratios, at the same time that a density field is being advected. A script to run this test this is provided in the case_studies repository. As this physics scheme needs to be solved with Backwards Euler, due to very sharp gradients in the ODEs governing the chemical interaction, the physics and dynamics need to be solved separately. Additionally, we prescribe the transporting velocity (a Nair Lauritzen setup) instead of having the velocity as a prognostic variable. A new SplitPrescribedTransport timestepper is written to enable this.