Refactor surface restart logic in FV3GFS_io.F90 to not use hard-coded indices.#605
Merged
Merged
Conversation
This was referenced Nov 18, 2022
joeolson42
approved these changes
Nov 18, 2022
Contributor
joeolson42
left a comment
There was a problem hiding this comment.
I'm not too familiar with this code, but the PR is well documented and the code looks clean.
Contributor
tanyasmirnova
left a comment
There was a problem hiding this comment.
@SamuelTrahanNOAA Very useful modifications to FV3GFS_io.F90 to remove hard-wired numbers! This subroutine used to be a culprit when some variables have to be added to the input or restart file. Thank you very much, Sam, for your tremendous work to make this subroutine more flexible.
tanyasmirnova
approved these changes
Nov 18, 2022
tanyasmirnova
approved these changes
Dec 8, 2022
tanyasmirnova
approved these changes
Dec 9, 2022
Collaborator
|
@SamuelTrahanNOAA Please update 'atmos_cubed_sphere' submodule to 16e659e5 hash from my branch. |
d0dbb21 to
94c955e
Compare
Collaborator
|
All tests are done on ufs-community/ufs-weather-model#1497. We can start merging in this pr. |
DusanJovic-NOAA
approved these changes
Dec 20, 2022
BrianCurtis-NOAA
approved these changes
Dec 20, 2022
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The surface restart uses hard-coded indices, which makes it difficult to add new restart variables, especially if different land surface schemes need different numbers of variables. This is problematic for developer @tanyasmirnova who has that exact problem.
The code now uses a variable
nt(number of things) it increments before copying to or from a slice of a surface field array. Most of the copies happen in copy_to_GFS_Data or copy_from_GFS_Data, which wraps this into a tight, vectorizable, loop, while also making the syntax concise.That means you'll see two constructs a lot.
Here, you see two columns: one increments nt, and one does the assignment. By having the two statements on one line, it is easy to spot if there's a missing
nt=nt+1:Most of the slices are copied like so:
where the
nt=nt+1and data copying logic is hidden behind the subroutine.There were a few odd copies, with bizarre indexing (-2:4) or ones with calculations. For those, the logic is made explicitly clear:
The code is also refactored to have the loops in an order that should be faster and easier to vectorize: nb, variable, k, ix.
When the code calls copy_to_GFS_Data or copy_from_GFS_Data, the k and ix loops are hidden behind subroutines:
Issue(s) addressed
fixes #604
Testing
hera.intel and hera.gnu ufs weather model regression tests