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

Add Python-wrapped versions of the wind transformation subroutines #345

Merged
merged 10 commits into from
Dec 20, 2022

Conversation

spencerkclark
Copy link
Member

@spencerkclark spencerkclark commented Dec 5, 2022

This PR adds Python-wrapped versions of the wind transformation subroutines contained in the dynamical core. This will allow us to more easily convert the horizontal winds between the D-grid and A-grid from within a Python-wrapper runfile.

Motivation

The primary motivation here is to enable us to more flexibly translate wind tendency updates made on the A-grid to the D-grid (e.g. to enable nudging toward A-grid winds or applying ML-predicted tendency corrections without modifying fields in the physics output state). For this we technically only need the A to D grid transformation, but the D to A grid transformation could potentially have use-cases as well (e.g. if we ever wanted to directly output A-grid wind nudging tendencies when we were nudging to D-grid winds).

Testing

The cubed_to_latlon and cubed_a2d subroutines are not exact inverses of each other, which means roundtrip tests are awkward (they require surprisingly large tolerances). As an alternative we instead leverage steps where these subroutines (or copies of these subroutines...) are already used within the dynamical core and verify that we are able to reproduce those results from within Python.

  • In the case of testing cubed_to_latlon this means verifying that we can recover Atm%ua and Atm%va from Atm%u and Atm%v.
  • In the case of testing cubed_a2d this means verifying that we can perform the physics increment of the dynamical core winds from within Python and get the same result as when it is done in fortran.

@spencerkclark spencerkclark marked this pull request as ready for review December 14, 2022 21:02
Copy link
Contributor

@oliverwm1 oliverwm1 left a comment

Choose a reason for hiding this comment

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

Thanks Spencer, these routines will be very useful as already evidenced by finding this bug for applying wind increments! Nice work!

FV3/wrapper/templates/_wrapper.pyx Outdated Show resolved Hide resolved
FV3/wrapper/templates/_wrapper.pyx Outdated Show resolved Hide resolved
FV3/wrapper/templates/dynamics_data.F90 Outdated Show resolved Hide resolved
def tearDown(self):
MPI.COMM_WORLD.barrier()

def test_transform_dgrid_winds_to_agrid_winds(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be interesting to also compare with the fv3net code used for D-grid -> A-grid transformation, which uses a precomputed 'wind rotation matrix'. Not needed for this PR, but would be helpful to know if this gives the same results.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I think some documentation of the round-trip between the D-grid -> A-grid and A-grid -> D-grid transformation would also be interesting, given that it's something we currently do for the wind nudging tendencies.

Comment on lines 114 to 117
# Deactivate fv_sg_adj for these tests. fv_sg_adj can otherwise introduce an
# additional momentum tendency on the A-grid winds that is not accounted for
# by the difference between the A-grid winds at the start of the physics and
# the A-grid winds at the end of the physics. This is just to make the
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this because the A-grid winds are not consistent with the 'true' dynamical core D-grid winds? Or some other reason? I'm not sure I follow.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason is a bit in the weeds; I can try to improve this comment. Note that u_dt and v_dt are global variables in the atmosphere.F90 module, so their state is carried between subroutines. It happens that they are reset to zero towards the end of the dynamics subroutine just before the call to fv_subgrid_z. fv_subgrid_z (if active) then modifies them, and this state is carried over into the physics update.

My reason for turning fv_subgrid_z off is just so that I don't need to worry about also recording and including the fv_subgrid_z increment in the A-grid to D-grid test. In principle if we did record it the test would still pass and we wouldn't have to worry about turning it off.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay thanks. I agree turning off for the purposes of this test is fine. Wanted to make sure this is not biting us elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

An implication (independent of this PR) is that the fv_subgrid_z tendencies are included in the bulk physics tendency diagnostics that we output. I don't think this is necessarily a concern, but something to be aware of.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was wondering about this, more with respect to the budget output from SHiELD.

Copy link
Member Author

Choose a reason for hiding this comment

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

Upon closer inspection it looks like fv_subgrid_z does not modify t_dt or q_dt at all (despite taking them as arguments), which I guess is consistent with the fact that I was able to approximately close the per-physics-component diagnostics budgets way back when without worrying about this. It instead modifies temperature and the tracers directly, so I believe those tendencies would not be included in our budget as it currently stands.

It only defers the u_dt and v_dt updates to the physics update step (perhaps understandably due to the requirement of the coordinate transformation).

Copy link
Contributor

@oliverwm1 oliverwm1 left a comment

Choose a reason for hiding this comment

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

I'm approving, since I think the question of vertical dimension order returned by the wrapper can be addressed separately, and the test here fixed if necessary.

Wind transformations are also valid on tendencies.  In general
the units of the outputs will match the inputs.
@spencerkclark spencerkclark merged commit 2ebcb0d into master Dec 20, 2022
@spencerkclark spencerkclark deleted the wrap_cubed_a2d branch December 20, 2022 21:27
spencerkclark added a commit to ai2cm/SHiELD-wrapper that referenced this pull request Nov 2, 2023
This PR ports the wind transformations added in ai2cm/fv3gfs-fortran#345 to SHiELD-wrapper. Conveniently this did not require any updates to SHiELD that were not already present in #1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants