Skip to content

Conversation

gmao-jstassi
Copy link
Contributor

Added collections for LDAS processing

  • bkg.inst1_2d_lfo
  • bkg.tavg1_2d_lfo

@gmao-jstassi gmao-jstassi added 0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) Skip Changelog labels Mar 20, 2025
@gmao-jstassi gmao-jstassi requested a review from a team as a code owner March 20, 2025 16:35
Copy link
Contributor

@rtodling rtodling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmao-jstassi which tag of the DAS have you started from? @saraqzhang and @gmao-rreichle is this a variable typically distributed among GMAO products? I ask to know about the consequences of commenting this out ...

@gmao-rreichle
Copy link
Contributor

@gmao-rreichle is this a variable typically distributed among GMAO products? I ask to know about the consequences of commenting this out ...

@rtodling: The "lfo_inst" and "lfo_tavg" Collections are routinely output in FP, M-2, GEOS-IT, etc as diagnostics from the Corrector segment. Their purpose is to drive offline land simulations. What we need here (for the coupled land-atm analysis) is the output of the same variables but from the Predictor segment (i.e., the bkg...lfo... Collections). What exactly do you want to comment out, or what do you see that is commented out but think it shouldn't be?

@gmao-jstassi
Copy link
Contributor Author

@rtodling, I started from your GEOSadas-5.41.1 tag and updated the HISTORY.rc.tmpl file with a version given to me by Sara. There shouldn't be any other changes.

… from these collections:

* tavg1_2d_lfo_Nx-
* tavg1_2d_lfo_Nx+-
* bkg.tavg1_2d_lfo
@gmao-jstassi
Copy link
Contributor Author

The HISTORY.rc.tmpl file was updated with version from Sara Zhang with SWLAND removed from these collections:

  • tavg1_2d_lfo_Nx-
  • tavg1_2d_lfo_Nx+-
  • bkg.tavg1_2d_lfo

Copy link
Contributor

@gmao-rreichle gmao-rreichle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saraqzhang, @rtodling, @gmao-jstassi: I added a few inline comments below. Please take a look.

steps:
- name: Checkout repo
uses: actions/checkout@v3.3.0
uses: actions/checkout@v4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in push-to-develop.yml and validate_yaml_files.yml aren't related to the intent of the PR. It doesn't matter to me if they're included, just pointing this out to avoid inadvertent changes and making sure nothing is out of sync.

'bkg.eta'
'bkg.sfc'
'cbkg.eta'
'bkg.inst1_2d_lfo'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we want these Collection names to be?
bkg.inst1_2d_lfo includes most but not all of the name of the standard (diagnostic) collection.
Using bkg.inst1_2d_lfo_Nx would be more consistent, where the "Nx" part expresses that the file is at the "native" resolution.
Since this is a "bkg" Collection, however, it may be better to keep only the essence of the standard collection name, as in bkg.lfo_inst1, or maybe even bkg.lfo_inst if we don't want to hardwire the 1-hour time step into the name.
My suggestion would be to use bkg.lfo_inst and bkg.lfo_tavg, which is probably most consistent with the other "bkg" Collection names.
(Note that GEOS-ESM/GEOSldas_GridComp#81 will need matching edits in LDAS_Forcing.F90.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

" Nx " should have not been in the lfo name since it conventionally indicates a lat/lon grid collection. We have had it in lfo Nx+- collection for the convenience ( existing name format in ldas) even though they output in CS resolution. We certainly can modify the names as Rolf suggested here, then there are several places needed to match in the adas PR #327 and in ldasGC PR#81.

tavg1_2d_lfo_Nx+-.end_time: >>>IOEBKGT<<< ,
tavg1_2d_lfo_Nx.fields: 'SLRSF' , 'SOLAR' , 'SWGDN' ,
'SWLAND' , 'SURFACE' ,
tavg1_2d_lfo_Nx+-.fields: 'SLRSF' , 'SOLAR' , 'SWGDN' ,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to keep the "lfo_Nx+-" Collections in the template. They are superseded by the "bkg.lfo" Collections. Both the "Nx+-" inst and tavg Collections should be removed.

bkg.tavg1_2d_lfo.end_time: >>>IOEBKGT<<< ,
bkg.tavg1_2d_lfo.fields: 'SLRSF' , 'SOLAR' , 'SWGDN' ,
'LWS' , 'IRRAD' , 'LWGAB' ,
'PCU' , 'SURFACE' , 'PRECCU' ,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, in the updated file specs of M21C, we now rename as follows:

PCU --> PRECRAINCU    (instead of PRECCU)
PLS --> PRECRAINLS    (instead of PRECLS)

We could make this change now in the bkg.lfo Collections, or we could stick with the older names (PRECCU, PRECLS) until the FP file specs are updated to the M21C conventions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not worry about this now.

@gmao-rreichle
Copy link
Contributor

@rtodling, I started from your GEOSadas-5.41.1 tag and updated the HISTORY.rc.tmpl file with a version given to me by Sara.

If this is merged into feature/rtodling/qrqstot_export_rename, how do we ensure that the changes make it into develop? Since the changes are trivially 0-diff, I suggest making a companion PR that merges the same changes into develop so this doesn't get lost as the repo moves forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0 diff trivial The changes in this pull request are trivially zero-diff (documentation, build failure, &c.) Skip Changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants