-
Notifications
You must be signed in to change notification settings - Fork 2
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
Run nesting and the new physics using the global-workflow #4
base: nest
Are you sure you want to change the base?
Conversation
add the global_nest physics into the global-workflow
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.
In an effort to simplify my changes, you have created something that will be unmaintainable. There are some major problems here. You need to put back in some of my changes.
sorc/build_ufs.sh
Outdated
#MAKE_OPT+=" -DFASTER=ON" #comment out this line for the global_nest runs | ||
|
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 should have been left in as a -f
command line option. Users should not edit the build scripts and uncomment things.
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.
-f command line option can be easily passed into build_ufs.sh but it is hard to be passed into build_all.sh
We will consider a full workflow instead of only the forecast part.
Users need to modify build_ufs.sh anyway to include the global_nest_v1 physics in the compiling time.
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.
In that case, it is best to use -DFASTER=ON as the default. It was designed to be both faster and safer than the defaults.
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.
I trust you. But I am not sure whether EMC will accept a default -DFASTER=ON. But we can try.
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.
The best option is to have -f in build_all.sh and have it passed down. If you don't want to do that, we should have it be -DFASTER=ON by default. Then EMC will tell us how to make it optional in the review.
ush/global_nest_patch.sh
Outdated
## patch link_workflow.sh | ||
sed -i -e 's#"hera").*#"hera") FIX_DIR="/scratch2/BMC/wrfruc/Guoqing.Ge/fix" ;;#' -e 's#versions/fix.ver#versions/fix.nest.ver#' $HOMEgfs/sorc/link_workflow.sh |
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.
Several problems here:
- Don't point to a user's personal directory.
- Including a script to patch the scripts makes code management much harder. Use "if" statements or command-line arguments ("--nest") instead.
- You should not replace the entire fix file hierarchy when all you are doing is adding a nested grid.
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 temporary for anyone interested to run this workflow. Otherwise, users will need to make those changes manually.
This file will not commit to the EMC repo.
The fix files will commit to the "authoritative“ location once they are accepted by EMC.
Yes, we can add a "--nest" option for link_workflow.sh
ush/global_nest_patch.sh
Outdated
declare -rx HOMEgfs | ||
|
||
## patch config.base | ||
sed -i -e 's#export BASE_CPLIC=.*#export BASE_CPLIC="/scratch2/BMC/wrfruc/Guoqing.Ge/ufs-ar/ICS/global-nest"#' -e 's/export DO_NEST="NO"/export DO_NEST="YES"/' $HOMEgfs/parm/config/gfs/config.base |
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 should be set by the setup_workflow script via a --nest option. Including a script to patch the scripts makes code management much harder. Relying on commented-out code is a bad practice.
Get rid of it and put in my original changes to setup_workflow.
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.
Do you mean we automatically set DO_NEST=YES and a new BASE_CPLIC location once we specify the "--nest" option?
I think whether to do nesting can be determined by a user through manually setting DO_NEST=YES in config.base under $EXPDIR
However, yes, we can make it happen automatically by the --nest option if needed.
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.
I'd prefer to reduce the manual edits of config.* files as much as possible.
ush/global_nest_patch.sh
Outdated
## patch build_ufs | ||
sed -i -e 's/FV3_GFS_v17_coupled_p8_ugwpv1"/FV3_GFS_v17_coupled_p8_ugwpv1,FV3_global_nest_v1"/' -e 's/^#MAKE_OPT+=/MAKE_OPT+=/' $HOMEgfs/sorc/build_ufs.sh |
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.
Three problems here:
- Relying on commented-out code is a bad practice.
- There is no reason to keep FV3_global_nest_v1 out of the compiled CCPP_SUITES. Adding a suite has no negative effect (other than a slight increase in compile time).
- Patching scripts makes code management more difficult.
- The nested workflow does not need -DFASTER=ON, so it should not be hard-coded to that. Only the C768 needs it, mostly as a limitation of Hera's nodes. We should have the freedom to change the build options without having to edit them in two places.
- Using a -f option in build_ufs.sh is simpler and better matches the current design.
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 a temporary file to avoid manual edits. It will not be committed to the authoritative repo.
Yes, as long as EMC accepts a new CCPP option in build_ufs.sh, we don't need this patch or user's edits.
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.
The temporary fix should be the cleaner one. Leave the changes in build_ufs.sh.
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.
Sure. I am fine with changing build_ufs.sh directly. I just stripped out all changes which will affect current GFS pre-v17 so as to make it ready for a PR to the EMC repo. But we can try your idea and modify if they have concerns. Thanks!
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.
Having a clean set of changes is more important than avoiding future discourse with EMC. It'll be difficult to review when everything is hidden behind sed commands in a ush/global_nest_patch.sh script. You need to remove that sedding script and make the changes in each individual file instead. It'll be both easier to maintain and easier to review.
versions/fix.nest.ver
Outdated
export am_ver=global-nest.20240419 | ||
export orog_ver=global-nest.20240419 | ||
export ugwd_ver=global-nest.20240419 |
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.
The vast majority of these files are not grid-specific. They're climatology files on their own grid. Do we really need our own copy of everything?
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 eliminates the need of "W384", "W768" etc. I think this simplifies the changes and make it easier to be accepted by EMC.
At the time I was working on this, I don't know what files are your new fix files.
We can either (1) make a new copy of every climatology file as the files size is not that big and acceptable;
or (2) create a new version but make those climatology files as links.
We can further discuss this in a tag up. Thanks!
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.
I see no other way to get rid of the W grids, so I agree we'll have to have our own fix files unless EMC comes up with a better option.
Co-authored-by: Samuel Trahan (NOAA contractor) <39415369+SamuelTrahanNOAA@users.noreply.github.com>
@SamuelTrahanNOAA Thanks for all the comments. I'm committing changes in this PR incrementally to the authoritative EMC repo and hope to complete it in the next few days. |
Your changes should not be merged to this repository while |
Sam, I don't plan to merge this PR as this is a temporary solution. I intended to use this PR to facilitate discussions. |
This PR bases on the latest EMC develop branch, add the nesting functionality and the FV3_global_nest_v1 CCPP physics suite to the workflow.
A quick instruction to run a global nest experiment and/or use the new physics is given at:
https://docs.google.com/document/d/15IOLJ_GwoKcbjjbsX2ITgbSIJmIJB-m8mRulRn1ikYg/edit