Fix Diagnostics of Diagnostics #452

Witt-D · 2023-10-10T14:16:21Z

I went through the thermodynamic diagnostics and added the self_setup_theromdyancs line where appropriate.
Additionatlly I added Toms hack fix for why the potential energy is failing

… diagnostic_minimal_fail

jshipton · 2023-10-10T15:39:53Z

examples/compressible/skamarock_klemp_nonhydrostatic.py

@@ -27,10 +27,10 @@
    tmax = dt
    dumpfreq = 1
 else:
-    nlayers = 10
-    columns = 150
-    tmax = 3600.


Did you mean to add all of these changes?

jshipton · 2023-10-10T15:40:27Z

examples/compressible/skamarock_klemp_nonhydrostatic.py

-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')]


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

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

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.

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

jshipton · 2023-10-10T15:41:40Z

gusto/diagnostics.py

-            # By default set this new field to the current value
-            # This may be overwritten if picking up from a checkpoint
-            field2.assign(field1)
+            self.init_field_set = False


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

tommbendall

Thanks for this Dan.

I've left a handful of comments, but actually I think there are some other things that we could do in this PR, that are harder to put in comments:

we should add a SteadyStateError of a diagnostic to another test (maybe Williamson 2)
we should add the PotentialEnergy diagnostic to an example to make sure that it's tested. Maybe the compressible DCMIP test?
while you've fixed the SteadyStateError of a diagnostic, we haven't fixed the Perturbation of a diagnostic. I think we should include a fix to that in this PR too.
a minor thing, but maybe you could update the name of the PR? I think it's more like "Fix diagnostics of diagnostics" -- you might have started by trying to making a minimum failing example but we don't want failing examples on main!

tommbendall · 2023-10-11T13:07:06Z

examples/compressible/skamarock_klemp_nonhydrostatic.py

-    columns = 150
-    tmax = 3600.
-    dumpfreq = int(tmax / (2*dt))
+    nlayers = 5


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

tommbendall · 2023-10-11T13:07:27Z

examples/compressible/skamarock_klemp_nonhydrostatic.py


 # 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)],


please keep the point data outputting in here for now!

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

tommbendall · 2023-10-11T13:09:26Z

examples/compressible/skamarock_klemp_nonhydrostatic.py

-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')]


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.

tommbendall · 2023-10-11T13:10:14Z

gusto/diagnostics.py

@@ -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)


Thanks for fixing this!

tommbendall · 2023-10-11T13:11:54Z

gusto/diagnostics.py

+
+            self.init_field_set = False
+
+            #HACK Defo not the best way to do this perhaps we want a flag for checkpoint or prognostic


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)

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

tommbendall

Thanks Dan! Good that the tests now pass.

A few more minor things to sort.

Do we also want to fix the Perturbation diagnostic as part of this PR?

PS: you might want to fix the typo in the name of the pull request!

examples/compressible/skamarock_klemp_nonhydrostatic.py

tommbendall · 2023-10-12T08:50:46Z

gusto/diagnostics.py

@@ -1033,12 +1033,30 @@ 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


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

tommbendall · 2023-10-12T08:51:36Z

gusto/diagnostics.py


        super().setup(domain, state_fields)

+    def compute(self):
+        if not self.init_field_set:


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

tommbendall · 2023-10-12T08:52:34Z

gusto/io.py

@@ -630,6 +630,10 @@ def pick_up_from_checkpoint(self, state_fields):
            self.output.checkpoint_pickup_filename = None
        else:
            raise ValueError("Must set checkpoint True if picking up")
+        # Prevent any steady-state diagnostics overwriting their original fields


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

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

tommbendall

Some typos to fix

gusto/diagnostics.py

Co-authored-by: tommbendall <[email protected]>

…eproject/gusto into diagnostic_minimal_fail

jshipton · 2023-11-01T10:34:54Z

gusto/timeloop.py

@@ -216,6 +216,7 @@ def set_reference_profiles(self, reference_profiles):
            set the reference field.
        """
        for field_name, profile in reference_profiles:
+            breakpoint()


this is why you're breaking the tests

tommbendall

This is very very nearly ready, you just need to fix the typos in a comment

gusto/timeloop.py

Co-authored-by: tommbendall <[email protected]>

tommbendall

Thanks for doing this Dan! Good to go now

Witt-D added 8 commits September 19, 2023 11:54

adjusted to check SUPG

75277e2

tried an empty assign fix

d6d1b62

Merge branch 'main' into diagnostic_minimal_fail

1cf50a2

testing diagnostic

fd8ad70

Merge branch 'main' of https://github.com/firedrakeproject/gusto into…

59af186

… diagnostic_minimal_fail

script for steadystate testing,

597ba6a

conflict fix

c6c1fa6

fixed diagnostics missing _setup_thermodyanmcs

0be6b0d

jshipton reviewed Oct 10, 2023

View reviewed changes

jshipton requested changes Oct 10, 2023

View reviewed changes

hack fix for prognostic

2d4798e

tommbendall requested changes Oct 11, 2023

View reviewed changes

added check for steadystate

7695709

Witt-D changed the title ~~Diagnostic minimal fail~~ Fix Diagnostics of Disagnosics Oct 11, 2023

Witt-D added 3 commits October 11, 2023 16:54

reverted test changes that got pushed accidently

53234f2

added steadystate check to williamson2

3996356

flake8 fixes and added a potential energy test

e2817f9

tommbendall requested changes Oct 12, 2023

View reviewed changes

Witt-D changed the title ~~Fix Diagnostics of Disagnosics~~ Fix Diagnostics of Diagnostics Oct 12, 2023

Witt-D added 2 commits October 12, 2023 10:33

reverted to original

0de497f

addressed commenting issues

efb8c90

tommbendall reviewed Oct 12, 2023

View reviewed changes

gusto/diagnostics.py Outdated Show resolved Hide resolved

gusto/diagnostics.py Outdated Show resolved Hide resolved

gusto/diagnostics.py Outdated Show resolved Hide resolved

Witt-D and others added 5 commits October 13, 2023 09:41

Update gusto/diagnostics.py

2132208

Co-authored-by: tommbendall <[email protected]>

Update gusto/diagnostics.py

63f4342

Co-authored-by: tommbendall <[email protected]>

Update gusto/diagnostics.py

053a9bc

Co-authored-by: tommbendall <[email protected]>

fix changes

ec1a2e5

Merge branch 'diagnostic_minimal_fail' of https://github.com/firedrak…

14396ad

…eproject/gusto into diagnostic_minimal_fail

jshipton reviewed Nov 1, 2023

View reviewed changes

Witt-D added 2 commits November 1, 2023 16:38

fixed linting and added perturbation as diagnostic to unsat buble

171fb16

flake 8 in examples2

0d90f99

diagnostic flake8

9ce0ecf

tommbendall requested changes Nov 7, 2023

View reviewed changes

gusto/timeloop.py Outdated Show resolved Hide resolved

Witt-D and others added 2 commits November 13, 2023 10:00

Update gusto/timeloop.py

725f1a9

Co-authored-by: tommbendall <[email protected]>

Merge branch 'main' into diagnostic_minimal_fail

c85cec0

tommbendall approved these changes Nov 13, 2023

View reviewed changes

tommbendall merged commit 6a1678b into main Nov 13, 2023
4 checks passed

tommbendall deleted the diagnostic_minimal_fail branch November 13, 2023 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Diagnostics of Diagnostics #452

Fix Diagnostics of Diagnostics #452

Witt-D commented Oct 10, 2023

jshipton Oct 10, 2023

jshipton Oct 10, 2023

Witt-D Oct 10, 2023

tommbendall Oct 11, 2023

Witt-D Oct 11, 2023

jshipton Oct 10, 2023

tommbendall left a comment

tommbendall Oct 11, 2023

tommbendall Oct 11, 2023

Witt-D Oct 11, 2023

tommbendall Oct 11, 2023

tommbendall Oct 11, 2023

tommbendall Oct 11, 2023

Witt-D Oct 11, 2023

tommbendall left a comment

tommbendall Oct 12, 2023

tommbendall Oct 12, 2023

tommbendall Oct 12, 2023

Witt-D Oct 12, 2023

tommbendall left a comment

jshipton Nov 1, 2023

tommbendall left a comment

tommbendall left a comment


		self.init_field_set = False

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

Fix Diagnostics of Diagnostics #452

Fix Diagnostics of Diagnostics #452

Conversation

Witt-D commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommbendall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommbendall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommbendall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommbendall left a comment

Choose a reason for hiding this comment

tommbendall left a comment

Choose a reason for hiding this comment