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.
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
Allow increments to be added for ocean and ice cycling. #1308
Allow increments to be added for ocean and ice cycling. #1308
Changes from 4 commits
311657d
364df4e
015e0bd
19c4748
46b1e7e
b04954b
55df592
acbeb76
6be7442
b8963ee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
MOM_diabatic_driver
or is it diabadic?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.
again
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.
Maybe somebody had a cold when writing that namelist @CoryMartin-NOAA
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.
So usually the MOM_input_template_* is a copy of what is in ufs-weather-model. Do these updates need a corresponding test in ufs-weather-model?
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.
Thanks for jumping in @JessicaMeixner-NOAA , I should have thought about adding you when I started modifying the mom namelist! Sorry.
Is there a MOM6 IAU test in the ufs-weather model?
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 think there is one here: https://github.com/ufs-community/ufs-weather-model/blob/develop/tests/tests/datm_cdeps_iau_gefs but it looks like the settings are different
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.
It seems close enough to me @JessicaMeixner-NOAA but up to you if you think it needs an update to the RT.
@aerorahul was trying to bring the naming nomenclature closer to what the atmosphere DA is doing, which does make sense.
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.
That sounds like a great idea to me too. I think it's important that we are testing in ufs-weather-model what we're actually going to be running and having consistency makes updating the model in the future easier. I believe there was talk at one point to actually point to the MOM_template files in ufs-eather-model instead of having a second copy of these, but I'm not sure if that's still going to be a future development for g-w or not. Either way, I just wanted to flag this as something to be aware of, but I don't think it should hold up this PR.
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.
Thanks @JessicaMeixner-NOAA
We can let this PR pass and I can open a follow up PR removing these files from global-workflow and using the ones from ufs-weather-model.
Would you be able to provide a branch of the ufs-weather-model that allows for this flexibility?
The ufs-weather-model RT parm/ files may need an update.
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.
We'd need the following:
I can do all of the above, but it will be a while before I personally have time to do this. Again, there's no reason this PR should not proceed.
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. 3. is on our radar.
We need atleast 1. for 3.
There is a lot more work in the ufs-weather-model than in the global-workflow to achieve this.
I can create issues in the global-workflow and ufs-weather-model and assign us respectively if that is ok?
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.
That sounds great. Just know this isn't anywhere near the top of my priority list at the moment.
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.
@JessicaMeixner-NOAA
#1312
ufs-community/ufs-weather-model#1613
have been created
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 a lint thing?
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.
@CoryMartin-NOAA , nope, that was an issue before you added a default to the other places where
DEBUG_WORKFLOW
is used. I'll remove that line.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 think this was a mistake to merge this change in... did anyone test? @jiaruidong2017 seems to be having an issue here
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.
why? is it not a
cold start
?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.
No, it's a
warm
start.