-
Notifications
You must be signed in to change notification settings - Fork 49
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
Feature: UFS Replay #372
Feature: UFS Replay #372
Conversation
@grantfirl Documentation added, section 5.5. I think this is ready to go otherwise. |
@dustinswales I have taken a brief look at the code but was waiting to test it myself as I see you are still making changes to the CI tests. Do you think this PR is stable enough to start a thorough review? |
@mkavulich Thanks! Yes, it's ready for review. Maybe this is all too much anyways? Since what we really need is to be alerted to UWM input/output changes that break the case generation script (we have another CI test that does the full workflow for DEPHY formatted input, which is what comes out of the case generation script). The functionality of this test is only useful if the data from the UWM is somewhat up-to-date. |
@grantfirl The new CI test to create the UFS-replay cases from staged UWM RT data is working, and passing. |
# Get command line arguments | ||
args = parser.parse_args() | ||
|
||
if (not args.dir): |
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 shouldn't be necessary due to the required=True
for the dir
argument.
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.
Good point. I will remove.
# Call UFS_IC_generator.py | ||
case_name = args.case_name +"_n" + str(pt).zfill(3) | ||
file_scminput = "../../data/processed_case_input/"+case_name+"_SCM_driver.nc" | ||
com = "./UFS_IC_generator.py -l " +str(lons[pt]) + " " + str(lats[pt]) + \ |
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.
Is this slow? Would it benefit from using multiprocessing, e.g. a solution like the execute
function in the run script?
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 refers to using the os.system(com)
serially, below.
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.
Yes, it's serially slow.
I will revisit this in the future.
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 looks good to me. I'm OK with it merged as-is and any comments addressed later. Thanks for pushing it across the finish line.
# | ||
tend_total[0,:] = stateREGRID["t_lay"][t+1,:] - stateREGRID["t_lay"][t,:] | ||
tend_remap[0,:] = stateREGRID["t_lay"][t,:] - stateNATIVE["t_lay"][t,:] | ||
dtdt_adv[t,:] = (tend_total[0,:] - tend_remap[0,:]) / dtime_sec |
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.
IMO, this method should be revised in the (near) future. Calculating tendencies this way includes the effect of physics already, which we want the physics in the SCM to represent. I think the "right" way to calculate advective tendencies, if we don't want to rely on having either the non-physics tendency terms or the all-physics tendency terms as "special" diagnostics (which most runs will not save due to space constraints), is to actually use the output winds with neighboring profiles of T, q with the upwind differencing scheme to derive them. I'll go ahead and start an issue so that we don't forget to do this.
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.
You are correct, I think of this as the quick and easier(dirtier) way.
I'm out of my lane when it comes to numerical recipes for computing advection, but it's not clear to me how using neighboring points gets around the implicit physics influence on the state? (I need to learn more)
The first is the functionality to create SCM cases using standard output from the UFS weather model. The changes contained here are built on-top of https://github.com/grantfirl/ccpp-scm/tree/UFS_forcing_20210203/scm. The dynamic tendencies to drive the SCM are derived in UFS_IC_generator.py. Then, along with the necessary initial conditions, these derived forcing terms are stored in DEPHYv1 format.