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

Refactor fv3atm history & restart to reduce redundant code. Add rrfs-sd and clm lake to quilt restart. #660

Merged
merged 31 commits into from
Jun 20, 2023

Conversation

SamuelTrahanNOAA
Copy link
Contributor

@SamuelTrahanNOAA SamuelTrahanNOAA commented May 25, 2023

Description

This is a major refactoring of the restart and history I/O code, which also adds CLM Lake and RRFS-SD to the quilting restart. It eliminates code duplication between read vs. write and quilt vs. non-quilt. Most notably, it merges the giant Sfcprop copy loops for sfc read, non-quilt write, and quilt write into a single function. With these changes, the code will be easier to maintain, and read vs. write bugs will be easier to find.

  1. The code in FV3GFS_io.F90 and FV3GFS_restart.F90 has been split into several files, divided by purpose.
    • High-level API:
      • fv3atm_restart_io.F90 = code to read or write restart files, whether by quilt or not
      • fv3atm_history_io.F90 = code to write history files, whether by quilt or not
    • Low-level API, only accessed by those two files:
      • fv3atm_oro_io.F90 = code that reads oro files.
      • fv3atm_clm_lake_io.F90 = all clm lake restart I/O
      • fv3atm_rrfs_sd_io.F90 = all rrfs-sd restart I/O and emissions reading.
      • fv3atm_sfc_io.F90 = all other sfc file restart I/O
      • fv3atm_common_io.F90 = common utilities.
  2. The quilt restart code can read and write CLM Lake and RRFS-SD scheme data.
  3. FV3GFS has been changed to fv3atm everywhere in the fv3atm repository, except output file metadata.
  4. The three massive loops that copy to and from Sfcprop are now a single subroutine, Sfc_io_transfer. This way, any differences between restart read and write will stand out prominently. (They're controlled by reading and warm_start flags.)
  5. Data storage is now inside derived types, eliminating the numerous module-level arrays and indexes.
  6. Quilt restart variables pass an axis index list instead of axis length (fix from @DusanJovic-NOAA)
  7. An unused CLM Lake axis is gone. This is needed to get the quilt restart to match, since it can't output axis variables for axes that are never used. (fix from @DusanJovic-NOAA)
  8. Comments for every subroutine, function, module, and derived type
  9. Removed a large block of code guarded by #ifdef JUNK
  10. Deleted unused variables.
  11. Consistent indentation in fv3*io.F90 files (emacs f90-mode default)

Issue(s) addressed

Testing

How were these changes tested?

New regression tests in ufs-community/ufs-weather-model#1769

What compilers / HPCs was it tested with?

Hera intel and gnu.

Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)

Yes, there are new regression tests.

Have the ufs-weather-model regression test been run? On what platform?

Yes, on hera.intel and hera.gnu.

Will the code updates change regression test baseline? If yes, why? Please show the baseline directory below.

Any code that writes out CLM Lake restart data will change because I removed an unneeded axis variable. This is necessary due to a limitation of the quilt restart: it can't write axis information for axes that are never used.

Please commit the regression test log files in your ufs-weather-model branch

Done.

Dependencies

None.

@SamuelTrahanNOAA
Copy link
Contributor Author

I'd like to ping:

  • @haiqinli whose RRFS-SD code I refactored from FV3GFS_io.F90 into FV3GFS_rrfs_sd_io.F90.
  • @tanyasmirnova, who is the other developer involved in CLM Lake.
  • @junwang-noaa who asked me to add RRFS-SD and CLM Lake restart functionality to the quilt

Copy link
Contributor

@tanyasmirnova tanyasmirnova left a comment

Choose a reason for hiding this comment

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

@SamuelTrahanNOAA Sam, thank you very much for your excellent work to make the I/O subroutine well-organized. It was the most confusing subroutine for me, and with your changes it would be easier to understand and add variables to I/O if needed.

io/FV3GFS_sfc_io.F90 Outdated Show resolved Hide resolved
@SamuelTrahanNOAA
Copy link
Contributor Author

At the behest of @DusanJovic-NOAA, I have refactored a bit more. The fv3atm_history_io.F90 contains all history file routines, and fv3atm_restart_io.F90 contains all restart file routines.

@SamuelTrahanNOAA SamuelTrahanNOAA changed the title Refactor FV3GFS restart to reduce redundant code. Add rrfs-sd and clm lake to quilt restart. Refactor fv3atm history & restart to reduce redundant code. Add rrfs-sd and clm lake to quilt restart. May 29, 2023
io/fv3atm_sfc_io.F90 Outdated Show resolved Hide resolved
@SamuelTrahanNOAA
Copy link
Contributor Author

It would be great if @climbfuji reviewed this, since I'm refactoring code he wrote.

@zach1221
Copy link
Collaborator

@tanyasmirnova would you be able to review this PR again? It appears your approval from two weeks ago dropped after a change.

@SamuelTrahanNOAA
Copy link
Contributor Author

would you be able to review this PR again? It appears your approval from two weeks ago dropped after a change.

No, github still shows it as being approved.

Screenshot_2023-06-15_19-14-25

@SamuelTrahanNOAA
Copy link
Contributor Author

I think it wants two approvals from people with reviewer access.

@zach1221
Copy link
Collaborator

@SamuelTrahanNOAA Yes, correct. We need one more approving review from a reviewer with write access.

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

This is great work @SamuelTrahanNOAA. Just one question for my understanding.

io/fv3atm_oro_io.F90 Show resolved Hide resolved
@FernandoAndrade-NOAA
Copy link

Regression tests for 1769 have completed successfully. This is ready for merging. @jkbk2004

@jkbk2004 jkbk2004 merged commit f9d68ad into NOAA-EMC:develop Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants