-
Notifications
You must be signed in to change notification settings - Fork 162
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
Refactor fv3atm history & restart to reduce redundant code. Add rrfs-sd and clm lake to quilt restart. #660
Conversation
…fv3atm into refactor-restart
I'd like to ping:
|
There was a problem hiding this 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.
…estart into fv3atm_restart_io.F90
At the behest of @DusanJovic-NOAA, I have refactored a bit more. The |
It would be great if @climbfuji reviewed this, since I'm refactoring code he wrote. |
@tanyasmirnova would you be able to review this PR again? It appears your approval from two weeks ago dropped after a change. |
I think it wants two approvals from people with reviewer access. |
@SamuelTrahanNOAA Yes, correct. We need one more approving review from a reviewer with write access. |
There was a problem hiding this 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.
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.FV3GFS_io.F90
andFV3GFS_restart.F90
has been split into several files, divided by purpose.fv3atm_restart_io.F90
= code to read or write restart files, whether by quilt or notfv3atm_history_io.F90
= code to write history files, whether by quilt or notfv3atm_oro_io.F90
= code that reads oro files.fv3atm_clm_lake_io.F90
= all clm lake restart I/Ofv3atm_rrfs_sd_io.F90
= all rrfs-sd restart I/O and emissions reading.fv3atm_sfc_io.F90
= all other sfc file restart I/Ofv3atm_common_io.F90
= common utilities.FV3GFS
has been changed tofv3atm
everywhere in the fv3atm repository, except output file metadata.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 byreading
andwarm_start
flags.)#ifdef JUNK
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.