Skip to content

Conversation

@saraqzhang
Copy link
Contributor

@saraqzhang saraqzhang commented Apr 25, 2025

Updates ldas_setup for improved integration of land-atm DAS config.

New functionality needed for revisions in GEOS-ESM/GEOSadas#352 (for coupled land-atm experiments).

Successfully 0-diff tested by @gmao-rreichle after 3d9121e


For the record: Spreadsheet with GEOSldas rc variables as discussed after ab9c3c3 (~7-8 May 2025)
LADAS_exeinp_variables_20250508T1113.xlsx


Left for future cleanup of ldas_setup:

@saraqzhang saraqzhang added enhancement New feature or request 0-diff labels Apr 25, 2025
@saraqzhang
Copy link
Contributor Author

@gmao-rreichle This PR contains the addition/modification to ldas_setup on top of ldasGC 2.0.0. When the coupling is on, it sets default values to a list of parameters. It also accepts a few more command line options that allow fvsetup pass adas information for the coupling. With these updates fvsetup will query where to get ldas initial conditions and pass them to ldas_setup. We only need one exeinp.txt ( work for both det and ens) . The modifications in fvsetup corresponding to this ldasGC PR are also implemented, but not yet in PR#327. The updated version has been tested.

…tup):

- renamed new command line arguments
- edited comments & help messages for clarity
- cleaned up white space and indent
@gmao-rreichle gmao-rreichle changed the title add setting defaults and more commdline options for ladas add default settings and more command line options for land-atm DAS May 2, 2025
Copy link
Collaborator

@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, @weiyuan-jiang: See inline comments below.
@weiyuan-jiang, the changes in this PR (#94) supplement what was merged in #81. Together, the two PRs modify ldas_setup by adding command line arguments for the benefit of setting up LDAS within the ADAS, where ldas_setup is called from within fvsetup (see GEOS-ESM/GEOSadas#327 and GEOS-ESM/GEOSadas#352).
@weiyuan-jiang, in addition to addressing the inline comments below, please take a look at the aforementioned PRs (#81 and #94) and make sure the modifications to ldas_setup are consistent with the overall design of ldas_setup. Thanks!

        edit lines responding to comments
@saraqzhang
Copy link
Contributor Author

added command line option to accept land BCs version info from fvsetup.

@saraqzhang
Copy link
Contributor Author

fvsetup only provide BCs version info, not the whole path. In exeinp we still need to specify BCS_PATH such as:
BCS_PATH: /discoer/nobackup/projects/gmao/bc_shared/fvInput/ExtData/esm/tiles/v11
if in ladas setup, ldas_setup will use the BCs version from fvsetup to replace the version "v11" in the string.

@gmao-rreichle
Copy link
Collaborator

@saraqzhang : I made a few more changes in an attempt to simplify things. I hope I didn't break anything. Please review my changes: https://github.com/GEOS-ESM/GEOSldas_GridComp/compare/29f77d8..7742280?w=1 (white-space changes are hidden in this link).
Also, don't we still need to update the sample files in GEOSldas_App/sample_config_files/LADAS?

@saraqzhang
Copy link
Contributor Author

@gmao-rreichle I pulled the updated version and ran setup test, it works. I have an updated version of exeinp.txt, I can put it to this branch served as a draft sample file in GEOSldas_App/sample_config_files/LADAS.
I also pushed updated command line args of ldas_setup in fvsetup to the branch where #352 sits. The setup test was done with fvsetup-> ldas_setup.

@gmao-rreichle
Copy link
Collaborator

I have an updated version of exeinp.txt, I can put it to this branch served as a draft sample file in GEOSldas_App/sample_config_files/LADAS.

Yes, please! And use "git rm ..." to delete the old sample files.

@saraqzhang
Copy link
Contributor Author

@gmao-rreichle I add a README file in the directory, it turned out no need a sample file, just the information on which parameters will be taken care of by fvsetup or by ldas_setup default.

@saraqzhang
Copy link
Contributor Author

it may no longer need this directory in the repo, or change the dir name ?

@gmao-rreichle
Copy link
Collaborator

@gmao-rreichle I add a README file in the directory, it turned out no need a sample file, just the information on which parameters will be taken care of by fvsetup or by ldas_setup default.

See 8a7c7fc#commitcomment-156483200

@gmao-rreichle
Copy link
Collaborator

@weiyuan-jiang : Many thanks for the review. I implemented the suggested changes in two commits:
https://github.com/GEOS-ESM/GEOSldas_GridComp/compare/8a7c7fc..f07f2e4?w=1
I hope I got everything right and resolved the comments for now. Please double-check, thanks!

@saraqzhang
Copy link
Contributor Author

@weiyuan-jiang Weiyuan, thanks for the update, I will make tests today.

@saraqzhang
Copy link
Contributor Author

@weiyuan-jiang @gmao-rreichle I made minor changes ( df1b1f3 ) and ran setup test with dummyexeinp and batinp.txt ( generated by previous sample run) in position arguments, the test successfully setup the exp.

@saraqzhang
Copy link
Contributor Author

the test is with ldas_cpl =1 and all command line inputs from fvsetup.

- cleaned up logic of processing resource manager inputs
- reverted to relative paths for rc files in ./etc
- documentation of new functionality
- updated language about compute nodes in sample batinp file (removed Skylake, added Milan)
- fixed indentation
@gmao-rreichle
Copy link
Collaborator

@saraqzhang, @weiyuan-jiang : I made more changes (4a1802e). Besides editing comments, I tried to clean up the logic of how resource manager inputs are processed. I also (tentatively) reverted back to relative paths for the location of the rc template files. With the absolute paths, the sample --exeinp command broke for me, and I think that ldas_setup should in any case be run from the ./install dir, so ../etc/ should always point to the rc template files. I have done some limited testing, but I can't promise that everything will work. We may have to iterate more.

@weiyuan-jiang
Copy link
Contributor

weiyuan-jiang commented May 22, 2025

@saraqzhang, @weiyuan-jiang : I made more changes (4a1802e). Besides editing comments, I tried to clean up the logic of how resource manager inputs are processed. I also (tentatively) reverted back to relative paths for the location of the rc template files. With the absolute paths, the sample --exeinp command broke for me, and I think that ldas_setup should in any case be run from the ./install dir, so ../etc/ should always point to the rc template files. I have done some limited testing, but I can't promise that everything will work. We may have to iterate more.

@gmao-rreichle Can you post the broken message? Or simply print out current_directory = os.path.dirname(file). There are two underline before and after file. it does not show here

@weiyuan-jiang
Copy link
Contributor

weiyuan-jiang commented May 22, 2025

@saraqzhang, @weiyuan-jiang : I made more changes (4a1802e). Besides editing comments, I tried to clean up the logic of how resource manager inputs are processed. I also (tentatively) reverted back to relative paths for the location of the rc template files. With the absolute paths, the sample --exeinp command broke for me, and I think that ldas_setup should in any case be run from the ./install dir, so ../etc/ should always point to the rc template files. I have done some limited testing, but I can't promise that everything will work. We may have to iterate more.

@gmao-rreichle Can you post the broken message? Or simply print out current_directory = os.path.dirname(file). There are two underline before and after file. it does not show here. Sara needs absolute path. Otherwise it is relative to fvsetup directory

@gmao-rreichle
Copy link
Collaborator

@gmao-rreichle Can you post the broken message? Or simply print out current_directory = os.path.dirname(file). There are two underline before and after file. it does not show here. Sara needs absolute path. Otherwise it is relative to fvsetup directory

@weiyuan-jiang, @saraqzhang : I realized that I was running ldas_setup from a directory that was not called "./bin" but had a longer name. (I just copied the key executables from a build by Matt's system tests.) In my testing, I then keep the rc template files in a directory ../etc/. The "full path" approach assumes that the current dir has a name that is 3 chars long current_directory[0:-4], and that the rc template files are in ../etc relative to the current path. After renaming my current directory to "bin", the "full path" approach works for me. But I don't see how this "full path approach" is different from just specifying the relative path. @saraqzhang : what is your error message when it fails with the relative path approach.

@weiyuan-jiang
Copy link
Contributor

@gmao-rreichle Can you post the broken message? Or simply print out current_directory = os.path.dirname(file). There are two underline before and after file. it does not show here. Sara needs absolute path. Otherwise it is relative to fvsetup directory

@weiyuan-jiang, @saraqzhang : I realized that I was running ldas_setup from a directory that was not called "./bin" but had a longer name. (I just copied the key executables from a build by Matt's system tests.) In my testing, I then keep the rc template files in a directory ../etc/. The "full path" approach assumes that the current dir has a name that is 3 chars long current_directory[0:-4], and that the rc template files are in ../etc relative to the current path. After renaming my current directory to "bin", the "full path" approach works for me. But I don't see how this "full path approach" is different from just specifying the relative path. @saraqzhang : what is your error message when it fails with the relative path approach.

I believe the current directory is where fvsetup sits when Sara runs fvsetup. So the relative path would not work.

@gmao-rreichle
Copy link
Collaborator

I believe the current directory is where fvsetup sits when Sara runs fvsetup. So the relative path would not work.

So fvsetup isn't run from the install directory? And if it isn't, why do we expect the rc files in [abspath]/etc/? Sara, please point me to a build of the DAS (with the integrated LDAS).

@weiyuan-jiang
Copy link
Contributor

I believe the current directory is where fvsetup sits when Sara runs fvsetup. So the relative path would not work.

So fvsetup isn't run from the install directory? And if it isn't, why do we expect the rc files in [abspath]/etc/? Sara, please point me to a build of the DAS (with the integrated LDAS).

The absolute path is ldas_setup's path.

@saraqzhang
Copy link
Contributor Author

@gmao-rreichle @weiyuan-jiang fvsetup runs ldas_setup as : $fvbin/ldas_setup .... $fvbin = /.../GEOSadas/install-SLES15/bin . During fvsetup run "pwd" could be in various different dir, but $fvbin/ldas_setup will guarantee the abs. path for etc/ is $fvbin with /bin replaced by /etc.

@gmao-rreichle
Copy link
Collaborator

I just pushed another commit c3ca2d5
Do you think this will work? I was trying to explain in comments what we're doing ,and it was difficult to explain [0:-4] without a lot of words

@weiyuan-jiang
Copy link
Contributor

I just pushed another commit c3ca2d5 Do you think this will work? I was trying to explain in comments what we're doing ,and it was difficult to explain [0:-4] without a lot of words

I think it should work

@saraqzhang
Copy link
Contributor Author

@gmao-rreichle @weiyuan-jiang The current version passed setup test ( with dummyexeinp and dummybatinp ) and run exp test.

@saraqzhang
Copy link
Contributor Author

I will make updates to fvsetup corresponding to the current ldas_setup and test the coupled setup from adas.

@gmao-rreichle
Copy link
Collaborator

gmao-rreichle commented May 22, 2025

@weiyuan-jiang, @saraqzhang : I added one more commit (0f220cb) in an attempt to clean up the logic of processing the default values from the GEOS_SurfaceGridComp.rc file. I did some limited testing but can't promise for sure it's flawless. When you get a chance, please test and/or take a look at the mods. Thanks!

PS: I just remembered that we have essentially the same logic in two places:

  1. In function _parseInputFile(...):
    def _parseInputFile(self, inpfile):
  2. In function _ProduceExeInput(...):
    for line in _f:

I suppose we can clean this up by creating a function for processing GEOS_SurfaceGridComp.rc. Maybe we can leave this for the bigger cleanup of ldas_setup.

@weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang, @saraqzhang : I added one more commit (0f220cb) in an attempt to clean up the logic of processing the default values from the GEOS_SurfaceGridComp.rc file. I did some limited testing but can't promise for sure it's flawless. When you get a chance, please test and/or take a look at the mods. Thanks!

PS: I just remembered that we have essentially the same logic in two places:

  1. In function _parseInputFile(...):
    def _parseInputFile(self, inpfile):
  2. In function _ProduceExeInput(...):
    for line in _f:

I suppose we can clean this up by creating a function for processing GEOS_SurfaceGridComp.rc. Maybe we can leave this for the bigger cleanup of ldas_setup.

Yes. I will clean that up

@gmao-rreichle gmao-rreichle marked this pull request as ready for review May 23, 2025 15:32
@gmao-rreichle gmao-rreichle requested a review from a team as a code owner May 23, 2025 15:32
@gmao-rreichle gmao-rreichle merged commit 9d67552 into develop May 27, 2025
17 of 18 checks passed
@gmao-rreichle gmao-rreichle changed the title add default settings and more command line options for land-atm DAS additional default settings and command line args for land-atm DAS May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-diff enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants