-
Notifications
You must be signed in to change notification settings - Fork 1
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
add LDAS_INCR to AGCM.rc.tmpl for land-atmos coupled das #288
Conversation
AGCM.rc.tmpl
Outdated
# 0 : no (default) | ||
# 1 : yes | ||
# --------------------------------------------------- | ||
LDAS_INCR: @LDAS_INCR |
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.
@saraqzhang @gmao-rreichle Just a question: should we use gcm_setup
to give this a value here? At the moment, AGCM.rc when untemplated will still have @LDAS_INCR
after an experiment is set up.
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 is, AGCM.rc
when created by gcm_setup
. Obviously, fvsetup
does its own 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.
@mathomp4 I see that gcm_setup and fvsetup use the same AGCM.rc.tmpl here. then we need to make #LDAS_ICNR @LDAS_INCR in AGCM.rc.tmpl here. In fvsetup we need to add 'uncomment" if LDAS_INCR is needed. @gmao-rreichle
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.
@saraqzhang Note: it's possible the GCM just doesn't care about this...so it doesn't matter. I think @sdrabenh is testing that now. If the GCM doesn't even know about LDAS_INCR
then this might be moot! I just wanted to make sure the GCM never tries to read this variable.
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, @mathomp4, for pointing out the gcm_setup connection. I agree that we should have a sensible value (or nothing at all) for LDAS_INCR:
after gcm_setup. I'm ok with either solution, ie, comment out LDAS_INCR in AGCM.rc.tmpl and then have fvsetup uncomment it, or having gcm_setup introduce a default of 0.
One thing to consider, though: If someone should want to run the GCM without the DAS but still ingest pre-computed LDAS_INCR data (such as in a replay), commenting out LDAS_INCR may be somewhat less attractive. Or maybe replays are entirely the domain of fvsetup, and nobody would use gcm_setup to do a replay? @saraqzhang, just something to think about in connection with the "replay" 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.
@gmao-rreichle
No worries. That is why we all do these tests. It's a lot to keep track of. I just tested with it commented out and everything seems happy:
Character Resource Parameter: CATCH_INTERNAL_RESTART_FILE:catch_internal_rst Using parallel NetCDF for file: catch_internal_rst Integer*4 Resource Parameter: LDAS_INCR:0
@saraqzhang would you mind pushing those changes?
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.
@gmao-rreichle I was not asked to test this PR. But yes, this was at the time I took a week off.
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.
@sdrabenh, @saraqzhang: I went ahead and committed the change, so should be good for GCM. Sara, I suppose now we'll need to modify fvsetup accordingly in a new GEOSadas 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.
@sdrabenh, @saraqzhang: With the latest commits, the PR is now trivially 0-diff, and in a silly way it's obsolete. We could conceivably address our needs without any changes to AGCM.rc.tmpl. However, since LDAS_INCR: is an rc variable in the GCM, I think it's best to have the commented-out line in AGCM.rc.tmpl even so, because it provides documentation, so I'd still recommend merging it into develop.
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.
@gmao-rreichle LDAS_INCR will be needed for future gcm replay from LADAS. it is good to have the addition now.
fvsetup sets the parameter LDAS_INCR to default 0 , user input can set it to 1 for land-atmos coupled das experiment.