-
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
Thermal sw #573
base: main
Are you sure you want to change the base?
Thermal sw #573
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.
I'm super excited about this. My main questions are whether we can move the compute_saturation
routine to be a method of the ThermalShallowWaterEquations
, and about the setup
method
@@ -147,6 +147,15 @@ class ShallowWaterParameters(Configuration): | |||
g = 9.80616 | |||
Omega = 7.292e-5 # rotation rate | |||
H = None # mean depth | |||
# Factor that multiplies the vapour in the equivalent buoyancy |
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 these parameters (beta2
, nu
and q0
) are all features of having moisture rather than the equivalent buoyancy, so could we omit the part about the equivalent buoyancy formulation in these comments?
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.
hmm the only time we get the parameters from here is when we use the equivalent buoyancy formulation... @nhartney does it make sense to get the moisture parameters from here in other cases? Are there more parameters that could go here?
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 it doesn't make sense to get the moisture parameters from here if we aren't using the equivalent buoyancy formulation, and I don't think there are more parameters that could go here. We need no moisture parameters for thermal shallow water (usual formulation), and if we are doing moist thermal shallow water then the moisture parameters will be passed through with the physics scheme.
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 actually am happy about keeping the parameters in here -- and maybe we should put other moist shallow water parameters in here too. We currently keep other moisture parameters in the parameters object for the compressible equations
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.
Oh yes, that's true. And I was wrong when I said that there are no other parameters that could go here - if we wanted to cover all our bases for moist shallow water options we'd need to add a beta1
parameter for convective feedback from moisture (to allow us to use the moist convective thermal and the moist convective pseudo-thermal equations). Then there's also the thresholds for phase changes and the proportions converted, but I feel that they don't belong with the equations though they are moist shallow water parameters.
∂D/∂t + ∇.(D*u) = 0, \n | ||
for Coriolis parameter 'f' and bottom surface 'b'. | ||
for Coriolis parameter 'f' and bottom surface 'B'. | ||
""" | ||
|
||
def __init__(self, domain, parameters, fexpr=None, bexpr=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.
Should we change bexpr
to Bexpr
or something like topog_expr
to help avoid confusion?
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.
yes
@@ -456,3 +654,11 @@ def __init__(self, domain, parameters, fexpr=None, | |||
|
|||
# Use the underlying routine to do a first linearisation of the equations | |||
self.linearise_equation_set() | |||
|
|||
|
|||
def compute_saturation(q0, nu, H, g, D, b, B=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.
Should we make this a method of the ThermalShallowWaterEquations
? It could return None
is there are no active tracers. I don't know if it would be better to be have a mixed field X
as the argument or the individual fields, but this would also give access to the parameters and B
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.
@nhartney in your branch this is in its own special file all by itself - I was wondering why that was? Is it used anywhere else?
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.
There wasn't a good reason to have it in a file on its own - I did it only because I didn't know where it should belong. I also use the function elsewhere to set up some of the examples.
∂u/∂t + (u.∇)u + f×u + b_e*∇(D+B) + 0.5*D*∇(b_e + beta_2 q_v)= 0, \n | ||
∂D/∂t + ∇.(D*u) = 0, \n | ||
∂b_e/∂t + u.∇(b_e) = 0, \n | ||
∂q_t/∂t + u.∇(q_t) = 0, \n |
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 matches the code below which requires us to have the q_t
moisture variable when using the equivalent buoyancy formulation. I had imagined we would do it with an ActiveTracer
object but this makes sense to me
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.
Yes, I wondered about how to do this... this way requires less work for the user and does the equivalent buoyancy formulation even mean anything without the moisture??
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 this makes sense too. I think the equivalent buoyancy formulation inherently has to have moisture, so to me it makes sense that that isn't something the user has to remember and include.
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.
Yes I agree with you both!
self.residual = self.generate_linear_terms(residual, self.linearisation_map) | ||
|
||
def setup(self): | ||
self.field_names = ['u', 'D'] |
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.
Do we need this setup
function? Could we just set self.field_names
and self.space_names
in the __init__
method?
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.
ugh, yeah, I don't like this either and there might be a better way of doing it... The problem is that the mixed function space is set up in the PrognosticEquationSet
__init__
method which gets passed field_names
and spaces
from the ShallowWaterEquations
__init__
method... so if we set those things in the __init__
method of ThermalShallowWaterEquations
then they get overwritten in ShallowWaterEquations
.
# -------------------------------------------------------------------- # | ||
# Linearise equations | ||
# -------------------------------------------------------------------- # | ||
# Add linearisations to equations | ||
self.residual = self.generate_linear_terms(residual, self.linearisation_map) | ||
|
||
def setup(self): |
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.
Do we need this setup
function? Could we just set self.field_names
and self.space_names
in the __init__
method?
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.
same comment as above - happy to talk about alternatives because I don't like this either!
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.
Ah I understand, this is because the ThermalShallowWaterEquations
inherit from the ShallowWaterEquations
, and we go through the whole normal process of setting up the shallow water equations and then modify them...
It does feel like unnecessary effort to avoid calling the parent __init__
method and duplicate all of the momentum equation in the ThermalShallowWaterEquations
class.
If we required the list of field names and the dictionary of function spaces to be @abstractproperty
s of all PrognosticEquationSet
s would that work?
@@ -0,0 +1,164 @@ | |||
from gusto import * |
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 realise this example has just been added here for now and maybe you're still planning to do this, but we will need to make it match the format of the other examples to ensure that it gets run through the testing -- i.e. it will need a __main__
method and use arg-parse so that it can be called from the command line with different arguments.
To be consistent with everything else, we'd also need a plotting script and a figure, although I think we could set up an issue and do that in a different PR
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 did just put this here for now but I can make those changes if we decide we want this example to stay.
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 it would be a nice example to have -- I think we should have something with the linear thermal equations
return u_zon*(f + tan(th)*u_zon/R) | ||
|
||
|
||
def Dval(X): |
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.
We could use our NumericalIntegral
code here, e.g. like this galewsky jet case study: https://github.com/firedrakeproject/gusto_case_studies/blob/main/case_studies/shallow_water/galewsky_jet.py#L128
We are also going to need to either adapt the existing |
q0 = parameters.q0 | ||
nu = parameters.nu | ||
beta2 = parameters.beta2 | ||
qsat_expr = compute_saturation(q0, nu, H, g, D, b_name, topog) |
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 we want b
here as the argument rather than b_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.
yes, good spot!
# add (moist) thermal source terms not involving topography - | ||
# label these as the equivalent pressure gradient term | ||
if equivalent_buoyancy: | ||
q0 = parameters.q0 |
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 q0
, nu
and beta2
need to all be a Firedrake Constant
.
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.
because they are now coming from the parameters class, they are already Constant
s (see line 81 of configuration.py)
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.
Oh yes I see, thanks!
…into thermal_sw
sat_expr = q0*H/(D) * exp(nu*(1-b/g)) | ||
else: | ||
sat_expr = q0*H/(D+topog) * exp(nu*(1-b/g)) | ||
return sat_expr |
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.
Is this an indentation mistake?
|
||
super().__init__(domain, parameters, | ||
equivalent_buoyancy=equivalent_buoyancy, | ||
fexpr=fexpr, bexpr=bexpr, space_names=space_names, |
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.
bexpr
is now topog_expr
in the init
method of the super class.
Separate out thermal shallow water and include equivalent buoyancy formulation.