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
Changes from 9 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
28 changes: 13 additions & 15 deletions examples/compressible/skamarock_klemp_nonhydrostatic.py
Original file line number Diff line number Diff line change
@@ -27,10 +27,10 @@
tmax = dt
dumpfreq = 1
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?

dumpfreq = int(tmax / (2*dt))
nlayers = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know exactly what point you started this branch from, but I think that there should be almost no changes to this file

columns = 30
tmax = 10*dt
dumpfreq = 1

# ---------------------------------------------------------------------------- #
# Set up model objects
@@ -44,22 +44,22 @@
# Equation
Tsurf = 300.
parameters = CompressibleParameters()
eqns = CompressibleEulerEquations(domain, parameters)
eqns = CompressibleEulerEquations(domain, parameters, u_transport_option='vector_advection_form')
print(f'Number of DOFs = {eqns.X.function_space().dim()}')

# I/O
points_x = np.linspace(0., L, 100)
points_z = [H/2.]
points = np.array([p for p in itertools.product(points_x, points_z)])
dirname = 'skamarock_klemp_nonlinear'
dirname = 'testing'

# Dumping point data using legacy PointDataOutput is not supported in parallel
if COMM_WORLD.size == 1:
output = OutputParameters(
dirname=dirname,
dumpfreq=dumpfreq,
pddumpfreq=dumpfreq,
dumplist=['u'],
point_data=[('theta_perturbation', points)],
Copy link
Contributor

Choose a reason for hiding this comment

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

please keep the point data outputting in here for now!

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 i had no intention of pushing the change to this example and will move to testing through the examples suggested above. For the reference when including point data the examples wouldn't run so will most likely revert a lot of the changes to skamarock and move to testing in Williams 2 and dcmip etc

dump_nc=True,
dump_vtus=False
)
else:
logger.warning(
@@ -69,13 +69,11 @@
output = OutputParameters(
dirname=dirname,
dumpfreq=dumpfreq,
pddumpfreq=dumpfreq,
dumplist=['u'],
)
dump_nc=True,
dump_vtus=False
)

diagnostic_fields = [CourantNumber(), Gradient("u"), Perturbation('theta'),
Gradient("theta_perturbation"), Perturbation('rho'),
RichardsonNumber("theta", parameters.g/Tsurf), Gradient("theta")]
diagnostic_fields = [CourantNumber(), Pressure(eqns), SteadyStateError('Pressure_Vt')]
Copy link
Contributor

Choose a reason for hiding this comment

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

Are any of the changes to this file meant to be added to this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes im not sure why the examples have been added as i was unaware that these had even been pushed from my local area?

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 it makes sense to add something like Pressure(eqns) and SteadyStateError('Pressure_Vt') to a test. I don't think this is the right test as we don't expect this test to preserve a steady state.

I think it would make more sense to add SteadyStateError('relative_vorticity') [not sure if this is the right name so please check!] to the Williamson 2 test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed the changes to the skamarock test and implemented it in Williamson2

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

# Transport schemes
31 changes: 28 additions & 3 deletions gusto/diagnostics.py
Original file line number Diff line number Diff line change
@@ -1033,12 +1033,33 @@ def setup(self, domain, state_fields):
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


self.init_field_set = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we can get rid of field2 - is that right?


#HACK Defo not the best way to do this perhaps we want a flag for checkpoint or prognostic
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline, I think we can avoid a hack like this by adding things to the else part of this statement:

field1 = state_fields(self.field_name1)
field2.assign(field1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed the fix to this by placing a check in the checkpointing routine to avoid it resetting as discuessed

if not self.name in ['rho', 'u', 'theta']:
self.init_field_set = True

# Attach state fields to self so that we can pick it up in compute
self.state_fields = state_fields
field2.assign(field1)

else:
self.init_field_set = True

super().setup(domain, state_fields)

def compute(self):
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."""
@@ -1200,7 +1221,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"))


@@ -1243,6 +1267,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"))