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

Split update_atmos_radiation_physics in SHiELD/atmos_model.F90 #30

Conversation

spencerkclark
Copy link
Member

@spencerkclark spencerkclark commented Oct 17, 2023

Description

Per our discussion today, this surfaces what is discussed in #27 as a PR for greater visibility (marking as a draft for now pending possible overlapping work by @JosephMouallem). It splits the update_atmos_radiation_physics subroutine into three parts:

  • update_atmos_pre_radiation
  • update_atmos_radiation
  • update_atmos_physics

The update_atmos_radiation_physics subroutine is retained and does exactly what it did before (so this does not change answers and requires no downstream code changes), but is written in a more modular fashion, exposing its steps as additional public subroutines in atmos_model_mod.

Fixes #27

xref: ai2cm/fv3gfs-fortran#317 for how we proceeded with this in FV3GFS.

How Has This Been Tested?

On top of the changes in #26, this has been tested in CI within the SHiELD-wrapper repository. For the test configuration there, after these changes we get bit-for-bit identical results.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included

@lharris4
Copy link
Contributor

@JosephMouallem could you weigh in with your progress or understanding of splitting up update_atmos_radiation_physics?

@JosephMouallem
Copy link
Contributor

These changes are Ok for me. I have created a whole new driver to accommodate the full coupler routines and coupling variables as in this branch

Copy link
Contributor

@lharris4 lharris4 left a comment

Choose a reason for hiding this comment

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

This splitting is surprisingly clean and shows a good way forward for how the full coupler would call SHiELD's different physics routines. Thanks.

@spencerkclark spencerkclark marked this pull request as ready for review October 19, 2023 12:08
@spencerkclark
Copy link
Member Author

spencerkclark commented Oct 19, 2023

Thanks for weighing in @JosephMouallem and for the review @lharris4. That's great that this split should not interfere with the coupling work. I've gone ahead and removed the "draft" designation from this PR.

@spencerkclark
Copy link
Member Author

A gentle ping on reviewing this PR @bensonr @laurenchilutti. Thanks!

SHiELD/atmos_model.F90 Outdated Show resolved Hide resolved
SHiELD/atmos_model.F90 Outdated Show resolved Hide resolved
SHiELD/atmos_model.F90 Show resolved Hide resolved
@spencerkclark spencerkclark force-pushed the split-update_atmos_radiation_physics branch from d409f1e to 68832a8 Compare November 1, 2023 14:25
Copy link
Contributor

@bensonr bensonr left a comment

Choose a reason for hiding this comment

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

@spencerkclark - thanks for making the changes

@laurenchilutti
Copy link
Contributor

I'll go ahead and merge this and the associated SHiELD_physics PRs

@laurenchilutti laurenchilutti merged commit 2ddc578 into NOAA-GFDL:main Nov 1, 2023
@spencerkclark spencerkclark deleted the split-update_atmos_radiation_physics branch November 1, 2023 15:18
spencerkclark added a commit to ai2cm/SHiELD-wrapper that referenced this pull request Nov 1, 2023
This PR updates the dycore, physics, and driver NOAA-GFDL submodules in this repo to their latest `main` branches.  Now that

- NOAA-GFDL/SHiELD_physics#31
- NOAA-GFDL/atmos_drivers#31
- NOAA-GFDL/atmos_drivers#30
- NOAA-GFDL/GFDL_atmos_cubed_sphere#302

have been merged, all the required features in SHiELD are in place to bring SHiELD-wrapper up-to-par with fv3gfs-wrapper.

Note that NOAA-GFDL/SHiELD_physics#31 made changes to the long names of some of the diagnostics output in our regression test, which required a checksum update.  

I have also taken the liberty to remove the disclaimer from the README that states that changes to SHiELD are required to bring SHiELD-wrapper up-to-par with FV3GFS.  That is no longer a limiting factor, and those remaining features are implemented in open PRs in this repo.
spencerkclark added a commit to ai2cm/SHiELD-wrapper that referenced this pull request Nov 3, 2023
This PR ports the changes in ai2cm/fv3gfs-fortran#317 to the SHiELD wrapper, which give us finer grained control over the steps of the time loop related to the physics.

It depends on NOAA-GFDL/atmos_drivers#30, which was merged upstream and incorporated into this repository in #11.
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.

Expose finer grained control over code in update_atmos_radiation_physics?
5 participants