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

Fix Diagnostics of Diagnostics #452

Merged
merged 25 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
75277e2
adjusted to check SUPG
Witt-D Sep 19, 2023
d6d1b62
tried an empty assign fix
Witt-D Sep 21, 2023
1cf50a2
Merge branch 'main' into diagnostic_minimal_fail
Witt-D Sep 25, 2023
fd8ad70
testing diagnostic
Witt-D Oct 6, 2023
59af186
Merge branch 'main' of https://github.com/firedrakeproject/gusto into…
Witt-D Oct 6, 2023
597ba6a
script for steadystate testing,
Witt-D Oct 9, 2023
c6c1fa6
conflict fix
Witt-D Oct 9, 2023
0be6b0d
fixed diagnostics missing _setup_thermodyanmcs
Witt-D Oct 10, 2023
2d4798e
hack fix for prognostic
Witt-D Oct 11, 2023
7695709
added check for steadystate
Witt-D Oct 11, 2023
53234f2
reverted test changes that got pushed accidently
Witt-D Oct 11, 2023
3996356
added steadystate check to williamson2
Witt-D Oct 11, 2023
e2817f9
flake8 fixes and added a potential energy test
Witt-D Oct 11, 2023
0de497f
reverted to original
Witt-D Oct 12, 2023
efb8c90
addressed commenting issues
Witt-D Oct 12, 2023
2132208
Update gusto/diagnostics.py
Witt-D Oct 13, 2023
63f4342
Update gusto/diagnostics.py
Witt-D Oct 13, 2023
053a9bc
Update gusto/diagnostics.py
Witt-D Oct 13, 2023
ec1a2e5
fix changes
Witt-D Oct 24, 2023
14396ad
Merge branch 'diagnostic_minimal_fail' of https://github.com/firedrak…
Witt-D Oct 24, 2023
171fb16
fixed linting and added perturbation as diagnostic to unsat buble
Witt-D Nov 1, 2023
0d90f99
flake 8 in examples2
Witt-D Nov 1, 2023
9ce0ecf
diagnostic flake8
Witt-D Nov 1, 2023
725f1a9
Update gusto/timeloop.py
Witt-D Nov 13, 2023
c85cec0
Merge branch 'main' into diagnostic_minimal_fail
tommbendall Nov 13, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion examples/compressible/dcmip_3_1_meanflow_quads.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,9 @@
dirname=dirname,
dumpfreq=dumpfreq,
)
diagnostic_fields = [Perturbation('theta'), Perturbation('rho'), CompressibleKineticEnergy()]
diagnostic_fields = [Perturbation('theta'), Perturbation('rho'),
CompressibleKineticEnergy(), PotentialEnergy(eqns)]

io = IO(domain, output, diagnostic_fields=diagnostic_fields)

# Transport schemes
Expand Down
8 changes: 4 additions & 4 deletions examples/compressible/skamarock_klemp_nonhydrostatic.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
else:
nlayers = 10
columns = 150
tmax = 3600.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to add all of these changes?

tmax = 3600
dumpfreq = int(tmax / (2*dt))

# ---------------------------------------------------------------------------- #
Expand Down Expand Up @@ -73,9 +73,9 @@
dumplist=['u'],
)

diagnostic_fields = [CourantNumber(), Gradient("u"), Perturbation('theta'),
Gradient("theta_perturbation"), Perturbation('rho'),
RichardsonNumber("theta", parameters.g/Tsurf), Gradient("theta")]
diagnostic_fields = [CourantNumber(), Gradient('u'), Perturbation('theta'),
Gradient('theta_perturbation'), Perturbation('rho'),
RichardsonNumber('theta', parameters.g/Tsurf), Gradient('theta')]
io = IO(domain, output, diagnostic_fields=diagnostic_fields)

# Transport schemes
Expand Down
2 changes: 1 addition & 1 deletion examples/compressible/unsaturated_bubble.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
checkpoint=False
)
diagnostic_fields = [RelativeHumidity(eqns), Perturbation('theta'),
Perturbation('water_vapour'), Perturbation('rho')]
Perturbation('water_vapour'), Perturbation('rho'), Perturbation('RelativeHumidity')]
io = IO(domain, output, diagnostic_fields=diagnostic_fields)

# Transport schemes -- specify options for using recovery wrapper
Expand Down
3 changes: 2 additions & 1 deletion examples/shallow_water/williamson_2.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@
dump_nc=True,
)

diagnostic_fields = [RelativeVorticity(), PotentialVorticity(),
diagnostic_fields = [RelativeVorticity(), SteadyStateError('RelativeVorticity'),
PotentialVorticity(),
ShallowWaterKineticEnergy(),
ShallowWaterPotentialEnergy(parameters),
ShallowWaterPotentialEnstrophy(),
Expand Down
30 changes: 29 additions & 1 deletion gusto/diagnostics.py
Original file line number Diff line number Diff line change
Expand Up @@ -1078,15 +1078,39 @@ def setup(self, domain, state_fields):
"""
# Check if initial field already exists -- otherwise needs creating
if not hasattr(state_fields, self.field_name2):
field1 = state_fields(self.field_name1)
field2 = state_fields(self.field_name2, space=field1.function_space(),
pick_up=True, dump=False)
# Attach state fields to self so that we can pick it up in compute
self.state_fields = state_fields
# The initial value for fields may not have already been set yet so we
# postpone setting it until the compute method is called
self.init_field_set = False
else:
field1 = state_fields(self.field_name1)
field2 = state_fields(self.field_name2, space=field1.function_space(),
pick_up=True, dump=False)
# By default set this new field to the current value
# This may be overwritten if picking up from a checkpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still want these deleted comments in here...
and this raises the point that we should make sure to comment our changes clearly so that we can still understand this change in 6 months time...

I suggest adding something like (you can use your own words!):

# The initial value for this field may not have been set yet, so we should postpone setting
# the initial field until the first time that the compute method is called

field2.assign(field1)
self.state_fields = state_fields
self.init_field_set = True

super().setup(domain, state_fields)

def compute(self):
# The first time the compute method is called we set the initial field.
# We do not want to do this if picking up from a checkpoint
if not self.init_field_set:
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment here along the lines of:

# The first time the compute method is called, we should ensure the initial field is set
# However we don't want to do this if picking up from a checkpoint

# Set initial field
full_field = self.state_fields(self.field_name1)
init_field = self.state_fields(self.field_name2)
init_field.assign(full_field)

self.init_field_set = True

super().compute()

@property
def name(self):
"""Gives the name of this diagnostic field."""
Expand Down Expand Up @@ -1248,7 +1272,10 @@ def setup(self, domain, state_fields):
state_fields (:class:`StateFields`): the model's field container.
"""
x = SpatialCoordinate(domain.mesh)
self.expr = self.rho_averaged * (1 + self.r_t) * self.parameters.g * dot(x, domain.k)
self._setup_thermodynamics(domain, state_fields)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this!

z = Function(self.rho_averaged.function_space())
z.interpolate(dot(x, domain.k))
self.expr = self.rho_averaged * (1 + self.r_t) * self.parameters.g * z
super().setup(domain, state_fields, space=domain.spaces("DG"))


Expand Down Expand Up @@ -1291,6 +1318,7 @@ def setup(self, domain, state_fields):
state_fields (:class:`StateFields`): the model's field container.
"""
u = state_fields('u')
self._setup_thermodynamics(domain, state_fields)
self.expr = 0.5 * self.rho_averaged * (1 + self.r_t) * dot(u, u)
super().setup(domain, state_fields, space=domain.spaces("DG"))

Expand Down
5 changes: 5 additions & 0 deletions gusto/io.py
Original file line number Diff line number Diff line change
Expand Up @@ -641,6 +641,11 @@ def pick_up_from_checkpoint(self, state_fields):
else:
raise ValueError("Must set checkpoint True if picking up")

# Prevent any steady-state diagnostics overwriting their original fields
Copy link
Contributor

Choose a reason for hiding this comment

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

Very picky, but it would be good to have a blank line above this comment to help illustrate that the next segment of code is doing something separate from the above bits of code

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 ive addressed the points regarding the example and some of the comments but was a bit uncertain on my explaining comments so they will be worth checking.

I think including perturbation in this will be a good idea too so will start having a dig around in that now

for diagnostic_field in self.diagnostic_fields:
if hasattr(diagnostic_field, "init_field_set"):
diagnostic_field.init_field_set = True

return t, reference_profiles, step, initial_steps

def dump(self, state_fields, t, step, initial_steps=None):
Expand Down
17 changes: 10 additions & 7 deletions gusto/timeloop.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,16 +227,19 @@ def set_reference_profiles(self, reference_profiles):
else:
raise ValueError(f'When initialising reference profile {field_name}'
+ ' the passed profile must be a Function')
# if field name is not prognostic we need to add it
ref.interpolate(profile)

# Assign profile to X_ref belonging to equation
if isinstance(self.equation, PrognosticEquationSet):
assert field_name in self.equation.field_names, \
f'Cannot set reference profile as field {field_name} not found'
idx = self.equation.field_names.index(field_name)
X_ref = self.equation.X_ref.subfunctions[idx]
X_ref.assign(ref)

if field_name in self.equation.field_names:
idx = self.equation.field_names.index(field_name)
X_ref = self.equation.X_ref.subfunctions[idx]
X_ref.assign(ref)
else:
# reference profile of a diagnostic
# warn user in case they made a typo
logger.warning(f'Setting reference profile for diagnostic {field_name}')
# Don't need to do anything else as value in field container has already been set
self.reference_profiles_initialised = True


Expand Down
Loading