-
Notifications
You must be signed in to change notification settings - Fork 0
updates for integrated setup of coupled land/atm DAS #81
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
Conversation
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 : I added a few inline comments that probably require further discussion at our next tag-up
modified: GEOSldas_App/ldas_setup modified: GEOSmetforce_GridComp/LDAS_Forcing.F90
modified: GEOSldas_App/ldas_setup
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, I added a few inline comments/suggestions/questions. Please take a look, thanks
modified: GEOSldas_App/GEOSldas_HISTdet.rc modified: GEOSldas_App/ldas_setup modified: GEOSmetforce_GridComp/LDAS_Forcing.F90
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 : I added a few inline comments, see below.
I took a look at the Surface GC code, if precip correction is on and the data file available for the period covering at least 12 hours starting from the rst time , the precip export (from Surface ) to lfo collection is the corrected quantity as long as AGCM runs the 12hr period, no dependency on whether it is in "corrector" or " predictor". |
modified: exeinp.txt.Hy4dEnVar.atmens modified: exeinp.txt.Hy4dEnVar.central
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: I tried to clean this up a bit. In a nutshell, the "bkg.lfo_*" files replace what used to be the "Nx+-" files, so there's no need to keep both definitions, and keeping both would only confuse things. Also, the intended logic is to determine "use_bkg" first, and then set GEOSgcm_defs accordingly. Note that "use_bkg" is set based on the optional extensions to the MET_TAG (which are identified by the double underscore, see subroutine parse_G5DAS_met_tag()). It should not be determined simply based on whether the string "bkg" appears in MET_TAG. Please take a look and let me know if everything looks ok. Thanks!
I pulled the clean-up update and ran a test, passed. |
modified: GEOSldas_App/GEOSldas_HISTens.rc modified: GEOSldas_App/ldas_setup modified: GEOSmetforce_GridComp/LDAS_Forcing.F90
@gmao-rreichle modifications and additions are pushed ( run tested ) |
…d lnd and lndfcstana collection defs to top of file, cleaned up indent (GEOSldas_HISTens.rc)
@weiyuan-jiang : Please look at the changes in ldas_setup and let us know if the new code is ok with the overall design of the script. 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.
@saraqzhang , see inline comments below
# ADAS_EXPDIR | ||
# ADAS_EXPID | ||
# EXP_ID | ||
# MET_TAG | ||
# BEG_DATE | ||
# GID |
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 : I'm wondering about the following exeinp rc variables:
LADAS_COUPLING
MET_PATH
MET_HINTERP
LAND_ASSIM
LANDASSIM_DT, LANDASSIM_T0
FIRST_ENS_ID
ENSEMBLE_FORCING
JOB_SGMT, NUM_SGMT
Essentially, bullet (2) above says that the user needs to make sure the values of the rc variables in their exeinp file match the ones shown in the "sample" file below. Since these values don't depend on anything that an LADAS user would change (assuming for now that the LADAS will always be run with 6-hour analysis windows), these values should be "forced" into ldas_setup by setup procedure. That is, if a user specifies them incorrectly, fvsetup and ldas_setup still use the correct value.
(1) One option to make this happen would be to add all of them to the list of command line args of ldas_setup, so the values could be hardwired into fvsetup. But that's a lot of additional command line args.
(2) Since the values don't depend on the user's LADAS config (unlike, say, ADAS_EXPID or MET_TAG), we could expand ldas_setup to look for them in a "DEFAULT" file whenever ldas_setup is used in the LADAS config. That is, we add two files to the source code (say, LADAS_exeinp_det.txt and LADAS_exeinp_ens.txt), and we hardcode the above rc variables into them. Then we need to teach ldas_setup to look for these variables in the "LADAS_exeinp*.txt" file. (We may need to have fvsetup furnish LADAS_COUPLING, so that ldas_setup knows whether to pick the "det" or "ens" version of the file.
(3) Or, alternatively, we could hardcode the values of these parameters directly into ldas_setup, and make sure that ldas_setup uses the hardcoded values whenever an LADAS experiment is configured.
The three options differ only in the location of the hardcoded values. In option (1), the hardcoded values live in fvsetup, in option (2) the hardcoded values live in "DEFAULT" files within the src code, and in option (3) the hardcoded values live in a ldas_setup [preferably in a separate function].
I think option (1) is least desirable because the hardcoded LDAS rc values would live in a distinct repo. Perhaps option (3) would be best because it avoids creating additional src files.
cc: @weiyuan-jiang
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 agree, I like the option (3) best. In the current PR version all things related to LADAS ( if coupled ) are scatted in ldas_setup, some info from ADAS, some info from exeinp example files. If we make a function in ldas_setup (1) hold the default values and (2) construct the parameters that depend on ADAS input ( currently done in the main code). ldas_setup calls it to complete the setting special for coupled case.
And we will no longer need special coupled exeinp example files in the repo.
cc: @weiyuan-jiang
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, @saraqzhang, sounds good. But we do need to keep the command-line inputs for EXP_ID etc because we can't predict the value and we don't want to have fvsetup write or edit a file that must then be read by ldas_setup.
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 will give it a try, adding a function and collect LADAS action items ( setting defaults and receiving fcsetup command-line inputs and assembling values) into one call. I may add more command line arguments in fvsetup for det or ens keyword, and resolutions.
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.
RESOLVING COMMENT FOR NOW SO PR CAN BE MERGED.
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.
REOPENING COMMENT AFTER MERGE TO MAKE IT EASIER TO IDENTIFY THE TASKS TO BE ADDRESSED IN A FUTURE PR.
…nsure backward compatibility with LDAS regression tests (to be cleaned up later) (LDAS_Forcing.F90)
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.
Approving and merging PR in its current form to expedite creation of a new release that is suitable for science experiments. Some tasks need to be completed at a later time under a separate PR, see introductory comment on present PR.
Updates are implemented to integrate ldas_setup into the coupled land/atm DAS setup procedure. In the coupled land/atmosphere config, the ADAS setup script (fvsetup) calls ldas_setup with optional command line inputs, so that LDAS experiments in the coupled land/atmosphere DAS are set up according to ADAS experiment specifications, incl. exp path, exp id, begin date and gid.
Changed nomenclature of met forcing files ("Nx+-" --> "bkg.lfo_*").
Successfully conducted LDAS regression tests (@gmao-rreichle, 14 April 2025)
[Tests with aggressive compilation failed the comparison for the (binary) catparam file. Specifically, variable cdcr1 had roundoff differences. This variable is the result of a calculation in preprocess_ldas_routines.F90.
UPDATE 18 April 2025: This non-zero-difference result was caused by my test running off a Cascade Lake chip, so some of the preprocessing was on Cascade Lake, whereas the automated nightly tests (and thus the Baseline) are run from discover-cron, which is Milan.]
To be done at a later time in a new PR:
Related PRs:
GEOS-ESM/GEOSadas#327
GEOS-ESM/GEOSgcm_App#707