-
Notifications
You must be signed in to change notification settings - Fork 167
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
Enable cycling atmosphere DA in 3DEnVar mode with coupled ensemble. #1718
Enable cycling atmosphere DA in 3DEnVar mode with coupled ensemble. #1718
Conversation
I will add more details to the description later. |
@@ -167,7 +167,7 @@ case "${CASE}" in | |||
export waveGRD='glo_500' | |||
;; | |||
"C96") | |||
export OCNRES=100 | |||
export OCNRES=500 |
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 not support OCNRES=100
as well?
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's supported, it's just not a default for any ATM resolution.
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.
OK, thank you for the clarification @WalterKolczynski-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.
We need to revisit this section in config.base
as more combinations are possible, but the limitation here is the bash configuration.
|
||
# TODO: Possibly need OCNRES_ENKF, ICERES_ENKF, WAVRES_ENKF too | ||
if [[ ${DO_OCN} == "YES" ]]; then | ||
case "${CASE_ENS}" in | ||
"C48") export OCNRES=500;; | ||
"C96") export OCNRES=100;; | ||
"C96") export OCNRES=500;; |
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.
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 set because the deterministic and the ensemble share the same ocean resolution, unlike the atmosphere where we have dual resolutions. I am sure we can support dual resolution ocean as well, but its not worth the hassle at this moment.
I haven't be requested as a reviewer, but other than the |
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.
Exiting stuff! Thanks @aerorahul , looks perfect to me.
I did not test your new feature
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.
LGTM. Approved.
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.
Generally looks good.
Co-authored-by: Walter Kolczynski - NOAA <[email protected]>
Co-authored-by: Walter Kolczynski - NOAA <[email protected]>
Co-authored-by: Walter Kolczynski - NOAA <[email protected]>
Co-authored-by: Walter Kolczynski - NOAA <[email protected]>
Co-authored-by: Walter Kolczynski - NOAA <[email protected]>
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.
shellcheck found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.
Automated global-workflow Testing Results:
|
Automated global-workflow Testing Results:
|
Automated global-workflow Testing Results:
|
Automated global-workflow Testing Results:
|
Automated global-workflow Testing Results:
|
Automated global-workflow Testing Results:
|
The error is:
@EdwardSafford-NOAA |
@TerrenceMcGuinness-NOAA @WalterKolczynski-NOAA |
Automated global-workflow Testing Results:
|
Automated global-workflow Testing Results:
|
@NeilBarton-NOAA |
addressed reviewer comments. requesting re-review.
This looks good to me. An important step. I don't have time to run it myself |
@aerorahul I've reviewed the CI failure and understand that the issue is that the location for RadMon output location is not specific to the PR/PR# when run as a CI test. This is also true for the other DA monitors, which define the output location as |
Description
This PR:
This PR also includes:
This PR does not:
Partially addresses #613
Type of change
How Has This Been Tested?
Checklist