fv3core uses the MetricTerms #16

rheacangeo · 2021-11-16T08:59:36Z

Purpose

Changes moved over from the feature/use_grid branch of fv3core -- ai2cm/fv3core#652
The fv3core/grid section now provided the functionality to create MetricTerms, the grid variables we need to run the model without reading them from serialized data on disk. When we want to test performance of new or larger datasets, it will be helpful to be able to initialize the grid without needing to first run the Fortran model to create the data.
This PR updates fv3core to make use of the MetricTerms class.

Create a GridData instance from a MetricTerms instance
Add a --compute_grid option to the regression tests to run the tests using a generated grid rather than serialized data from the Grid-Info savepoint. Currently this can only be applied to Parallel tests, and is only enabled for FVDynamics. Tracer2D1L fails (metric diff of 1e-9) because the computed GridData does not match the Fortran version bit-for-bit.
Added running FVDynamics both with and without --compute_grid to the parallel_regression jenkins tests
use ak, bk, ptop and ks from the grid_data rather than from serialized input data
update the performance scripts and wrapper runfiles to call FVDynamics with its updated API and not use the Grid-Info savepoint
Moved grid constants as attributes to MetricTerms to avoid circular imports
removed all instances of "spec.grid." to enable running the performance script without the Grid-Info savepoint. Added TODOs to revisit and maybe compute those derived grid terms closer to where they are used
added lon/lat to MetricTerms. Although perhaps redundant, I imagine these would be common variables someone would want from this class. Use them in a2b_ord4 and it makes that code more readable. Updated the fortran changelog to reflect this.
add computations of the coriolis parameters f0 and fC to the model (part of the 'fortran 'gridstruct' that gets computed in state initialization rather than in the grid. here computing them in the init of the C_SW and D_SW instances that use them.

…d_data.ks

rheacangeo · 2021-11-16T16:33:05Z

launch jenkins

rheacangeo · 2021-11-16T16:34:56Z

fv3core/fv3core/grid/generation.py

-        min_area = self._np.min(self.area.view[:])
-        max_area = self._np.max(self.area.view[:])
-        min_area_c = self._np.min(self.area_c.view[:])
-        max_area_c = self._np.max(self.area_c.view[:])
-
+        min_area = self._np.min(self.area.storage[3:-4, 3:-4])[()]
+        max_area = self._np.max(self.area.storage[3:-4, 3:-4])[()]
+        min_area_c = self._np.min(self.area_c.storage[3:-4, 3:-4])[()]
+        max_area_c = self._np.max(self.area_c.storage[3:-4, 3:-4])[()]


Otherwise cupy.min on a quantity view returns a cupy array, while numpy returns a scalar as expected.

rheacangeo · 2021-11-16T16:35:58Z

fv3core/fv3core/grid/geometry.py

@@ -601,119 +601,6 @@ def calculate_grid_a(z11, z12, z21, z22, sin_sg5):
    return a11, a12, a21, a22


-def edge_factors(


this calculation is moved to a2b_ord4

Basically did this due to a comment in a2b that the edge factors should be computed locally in that module because it is the only place it is used. Since I was deleting the trigger to do this work (calling spec.grid), I thought I should do the work. After our discussion, have reverted this change, and a future PR may put them in their own class structure to make it easier to read.

…tive to the fortran code

rheacangeo · 2021-11-16T18:24:12Z

launch jenkins

rheacangeo · 2021-11-16T21:18:48Z

launch jenkins

rheacangeo · 2021-11-16T22:10:01Z

Current failures may not be specific to this branch, others have reported similar in other branches. This is specific to the gtc backends. This is the error:
fv3core/fv3core/.gt_cache/py38_1013/gtcgtcpu_ifirst/fv3core/utils/stencil/copy_stencil/m_copy_stencil__gtcgtcpu_ifirst_0224270a4e_pyext.cpython-38-x86_64-linux-gnu.so: cannot open shared object file: No such file or directory

rheacangeo · 2021-11-16T23:58:09Z

launch jenkins

…e into feature/use_new_grid_in_dycore

oelbert

This looks really good! I think there's a lot in here that maybe should be split into separate PRs, like moving the edge factors into a2b_ord4 or putting the grid constants back into global_constants (not sure if either of those are necessary, but that's another reason to split them into separate PRs). I have a few more nits about comments and function naming, but overall really good.

oelbert · 2021-11-17T21:53:20Z

fv3core/examples/standalone/runfile/acoustics.py

@@ -39,18 +39,6 @@ def initialize_serializer(data_directory: str, rank: int = 0) -> serialbox.Seria
    )


-def read_grid(serializer: serialbox.Serializer, rank: int = 0) -> Grid:


Do we want to keep this functionality just in case?

We might want to run other test cases besides baroclinic, and even from a serialized grid

oelbert · 2021-11-18T17:57:40Z

fv3core/examples/standalone/runfile/dynamics.py

        communicator = util.CubedSphereCommunicator(mpi_comm, partitioner)
+        # generate the grid
+        grid = spec.make_grid_with_data_from_namelist(


oelbert · 2021-11-18T18:01:57Z

fv3core/fv3core/utils/global_constants.py

+TILE_DIM = "tile"
+CARTESIAN_DIM = "xyz_direction"
+N_TILES = 6
+RIGHT_HAND_GRID = False


Why are we re-combining these into global_constants? This was one of Jeremy's asks for the initial grid PR in order to keep the namespaces separate.

I ended up with a circular import, is there a reason these can't live next to MetricTerms?

oelbert · 2021-11-18T18:02:34Z

fv3core/fv3core/utils/grid.py

@@ -7,22 +7,15 @@

 import fv3core.utils.global_config as global_config
 import fv3gfs.util
+from fv3core.grid import MetricTerms
+from fv3core.utils.global_constants import CARTESIAN_DIM, LON_OR_LAT_DIM, TILE_DIM


See comment on global_constants

oelbert · 2021-11-18T18:05:36Z

fv3core/fv3core/grid/generation.py

+            dims=self.area.dims,
+            units="m^-1",
+            gt4py_backend=self.area.gt4py_backend,
+        )


Good catch on these attributes!

You are ok with having these here?

Yeah, I just completely neglected putting them in before, even though I should have

oelbert · 2021-11-18T18:58:12Z

fv3core/tests/savepoint/conftest.py

@@ -565,3 +579,8 @@ def __init__(self, func, origin, domain, *args, **kwargs):
 @pytest.fixture()
 def python_regression(pytestconfig):
    return pytestconfig.getoption("python_regression")
+
+
+@pytest.fixture()


This is great, thanks for adding it!

oelbert · 2021-11-18T19:17:48Z

fv3core/fv3core/stencils/updatedzd.py

    ):
        """
        Args:
            grid: fv3core grid object
            namelist: flattened fv3gfs namelist
            dp0: air pressure on interface levels, reference pressure
                can be used as an approximation
-            column_namelist: ???


always happy to remove ? from docstrings

oelbert · 2021-11-18T19:19:30Z

fv3core/fv3core/testing/translate_fvdynamics.py

        self.dycore = fv_dynamics.DynamicalCore(
            comm=communicator,
-            grid_data=spec.grid.grid_data,
+            grid_data=grid_data,  # grid_data,


Do we need this comment or the one below for damping data? It seems self-explanatory from the names.

good catch lol

oelbert · 2021-11-18T19:41:14Z

fv3core/fv3core/utils/stencil.py

@@ -17,6 +17,7 @@
 )

 import gt4py
+import numpy


I see why we're doing this double import, but is there another way to do this? It feels really awkward

no this was not on purpose, good catch! Also removing now that a2b is moved back

oelbert · 2021-11-18T19:44:36Z

fv3core/tests/savepoint/translate/translate_grid.py

@@ -1632,9 +1633,20 @@ def compute_parallel(self, inputs, communicator):
        in_state = self.state_from_inputs(inputs)
        grid_generator._grid.data[:] = in_state["grid"].data[:]
        grid_generator._agrid.data[:] = in_state["agrid"].data[:]
+        a2b = AGrid2BGridFourthOrder(


Yeah this feels very awkward here instead of keeping the edge factors inside the grids

So we're sticking with it?

…hrough the grid_data object instead. A future PR may orgnaize these to be computed at a different stage

… launching simulations that are not baroclinic

…in_dycore

… on the MetricTerms class. To avoid a circular import with MirrorGrid, changed RIGHT_HAND_GRID to be an argument in the context of mirroring, set to to a constant in out calls of it

oelbert

3 extremely minor nitpicks and one question, otherwise looks good to go

fv3core/examples/standalone/runfile/acoustics.py

fv3core/fv3core/grid/generation.py

oelbert · 2021-11-22T17:14:26Z

fv3core/fv3core/grid/generation.py

+            dims=self.area.dims,
+            units="m^-1",
+            gt4py_backend=self.area.gt4py_backend,
+        )


Yeah, I just completely neglected putting them in before, even though I should have

fv3core/fv3core/grid/generation.py

oelbert · 2021-11-22T17:21:20Z

fv3core/fv3core/stencils/c_sw.py

@@ -376,6 +376,17 @@ def initialize_delpc_ptc(delpc: FloatField, ptc: FloatField):
        ptc = 0.0


+def compute_fC(stencil_factory: StencilFactory, lon: FloatFieldIJ, lat: FloatFieldIJ):


oelbert · 2021-11-22T17:29:52Z

fv3core/tests/savepoint/translate/translate_grid.py

@@ -1632,9 +1633,20 @@ def compute_parallel(self, inputs, communicator):
        in_state = self.state_from_inputs(inputs)
        grid_generator._grid.data[:] = in_state["grid"].data[:]
        grid_generator._agrid.data[:] = in_state["agrid"].data[:]
+        a2b = AGrid2BGridFourthOrder(


So we're sticking with it?

Co-authored-by: Oliver Elbert <[email protected]>

rheacangeo · 2021-11-22T18:49:37Z

launch jenkins

oelbert

Awesome!

rheacangeo · 2021-11-22T19:47:26Z

launch jenkins

rheacangeo · 2021-11-22T19:48:31Z

launch jenkins

rheacangeo · 2021-11-22T21:22:38Z

launch jenkins

changes moved over from the feature/use_grid branch of fv3core

594b934

rheacangeo marked this pull request as draft November 16, 2021 08:59

rheacangeo requested a review from oelbert November 16, 2021 09:00

replace state.ptop and state.ks with self.grid_data.ptop and self.gri…

abf235b

…d_data.ks

rheacangeo commented Nov 16, 2021

View reviewed changes

rheacangeo added 2 commits November 16, 2021 08:43

update fortran changelof to reflect naming changes in the dycore rela…

ae1e500

…tive to the fortran code

linting bites again

693b10e

Merge branch 'main' into feature/use_new_grid_in_dycore

8341c1c

rheacangeo marked this pull request as ready for review November 17, 2021 00:46

rheacangeo added 2 commits November 18, 2021 08:19

fixing merge with master issues

5ac36ba

Merge branch 'feature/use_new_grid_in_dycore' of github.com:ai2cm/pac…

3d78614

…e into feature/use_new_grid_in_dycore

oelbert reviewed Nov 18, 2021

View reviewed changes

rheacangeo and others added 12 commits November 18, 2021 13:16

reverting computing edge_factors as A2B class methods, passing them t…

3414fb2

…hrough the grid_data object instead. A future PR may orgnaize these to be computed at a different stage

adding comments to other local grid variables

208619f

keeping the acoustics read grid method, which could come in handy for…

881dac6

… launching simulations that are not baroclinic

remove redendant comments

8db3f36

removing the np proprty from StencilConfig

5f8309c

adding docstrings for the coriolis parameters

fb706ce

fixes in response to review comments

2eacf63

Merge remote-tracking branch 'origin/main' into feature/use_new_grid_…

21ef672

…in_dycore

moving grid constants out of global_constant and adding as attributes…

5c6910d

… on the MetricTerms class. To avoid a circular import with MirrorGrid, changed RIGHT_HAND_GRID to be an argument in the context of mirroring, set to to a constant in out calls of it

Merge branch 'main' into feature/use_new_grid_in_dycore

d7f011a

fix merge conflicts

361ca16

switch back to edge factors being tested using MetricTerms

d2aed91

oelbert reviewed Nov 22, 2021

View reviewed changes

rheacangeo and others added 3 commits November 22, 2021 10:33

Update fv3core/examples/standalone/runfile/acoustics.py

6c0f5cb

Co-authored-by: Oliver Elbert <[email protected]>

Update fv3core/fv3core/grid/generation.py

14c8aab

Co-authored-by: Oliver Elbert <[email protected]>

Update fv3core/fv3core/grid/generation.py

8b3fd4c

Co-authored-by: Oliver Elbert <[email protected]>

oelbert approved these changes Nov 22, 2021

View reviewed changes

rheacangeo enabled auto-merge (squash) November 22, 2021 19:48

rheacangeo merged commit d234c3b into main Nov 22, 2021

rheacangeo deleted the feature/use_new_grid_in_dycore branch November 22, 2021 22:05

		@@ -601,119 +601,6 @@ def calculate_grid_a(z11, z12, z21, z22, sin_sg5):
		return a11, a12, a21, a22


		def edge_factors(

		@@ -39,18 +39,6 @@ def initialize_serializer(data_directory: str, rank: int = 0) -> serialbox.Seria
		)


		def read_grid(serializer: serialbox.Serializer, rank: int = 0) -> Grid:

		@@ -376,6 +376,17 @@ def initialize_delpc_ptc(delpc: FloatField, ptc: FloatField):
		ptc = 0.0


		def compute_fC(stencil_factory: StencilFactory, lon: FloatFieldIJ, lat: FloatFieldIJ):

fv3core uses the MetricTerms #16

fv3core uses the MetricTerms #16

Conversation

rheacangeo commented Nov 16, 2021 • edited Loading

Purpose

rheacangeo commented Nov 16, 2021

rheacangeo Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rheacangeo Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

rheacangeo commented Nov 16, 2021

rheacangeo commented Nov 16, 2021

rheacangeo commented Nov 16, 2021 • edited Loading

rheacangeo commented Nov 16, 2021

oelbert 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

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

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

oelbert 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

rheacangeo commented Nov 22, 2021

oelbert left a comment

Choose a reason for hiding this comment

rheacangeo commented Nov 22, 2021

rheacangeo commented Nov 22, 2021

rheacangeo commented Nov 22, 2021

rheacangeo commented Nov 16, 2021 •

edited

Loading

rheacangeo Nov 16, 2021 •

edited

Loading

rheacangeo Nov 18, 2021 •

edited

Loading

rheacangeo commented Nov 16, 2021 •

edited

Loading