Skip to content
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

NEON FATES capabilities #1932

Merged
merged 44 commits into from
Feb 2, 2023
Merged

NEON FATES capabilities #1932

merged 44 commits into from
Feb 2, 2023

Conversation

adrifoster
Copy link
Collaborator

@adrifoster adrifoster commented Jan 10, 2023

Description of changes

Small updates to facilitate creation, modification, and use of FATES-usable (i.e. 16-PFT) NEON surface data files and user-mods for FATES NEON cases.

Specific notes

I updated neon_surf_wrapper.py and modify_singlept_site_neon.py to include a --16pft argument that will create and/or modify the 16-PFT versions of the surface datasets, as well as a --mixed flag to the neon_surf_wrapper.py which tells subset_data to not overwrite the surfae dataset to be just 100% 1 PFT. Right now FATES can't run with the 78-PFT versions and we aren't sure when this will get fixed so this is a somewhat temporary change.

The --16pft argument just modifies the file names in the find_surffile function and also makes sure we don't set the surface file to be just one dominant PFT.

I also created a set of new user mods in cime_config/usermods_dirs/NEON/FATES. These are the exact same user_mods files as in the big-leaf version but with the surface data file name (in the user_nl_clm file) updated to reference the new files I created with these scripts (in /glade/p/cesmdata/inputdata/lnd/clm2/surfdata_map/NEON/16PFT_mixed).

The lines about lightning files were also deleted as these are not allowed in a FATES run (@ekluzek I'm not sure if this is also something we need to update in the FATES code to run NEON?)

Finally the hist_fincl2 names were updated to FATES-specific ones (again something we need to think about @ekluzek).

A big issue here is the replication of all the directories for the FATES user mods - What do we think?
The main problem is that when we reference a NEON site user_mods we reference the folder (i.e. /user_mods/NEON/ABBY) and then that user_mods references the ../defaults in the include_user_mods file. So for the FATES-specific defaults files, we need everything to be in its own encompassing folder.

Can we somehow modify the way include_user_mods is read to allow for variables? My idea would be for the include_user_mods to say something like:

../defaults_${version}

And then in shell_commands we would set up version=fates or version=BL or something. We could then have 2 defaults folders (defaults_fates, defaults_BL). @billsacks what do you think? This would I think be the least duplicative...

Contributors other than yourself, if any: @ekluzek, @billsacks, @TeaganKing

CTSM Issues Fixed: Working towards #1609

Are answers expected to change? No

Any User Interface Changes? No

Testing performed, if any:

Tested the ABBY NEON site and seemed to run fine, can test all NEON sites if needed.

Will run aux_clm and fates test suites - should probably create a test for FATES-NEON?

@adrifoster adrifoster changed the title Neon fates NEON FATES capabilities Jan 10, 2023
@adrifoster
Copy link
Collaborator Author

In talking with @jkshuman it might be good to also push this through before the winter meeting so that we can have FATES/NEON at least functional, if not the optimal method of setting up the cases. We can then create a new PR to get rid of all the hacky user-mods stuff.

@jkshuman
Copy link
Contributor

Agree with @adrifoster that being able to present this NEON FATES functionality at the winter meeting would be useful. Then further optimization can happen behind the scenes, since the functionality will not change.

@wwieder
Copy link
Contributor

wwieder commented Jan 10, 2023

+1, it would be great to illustrate these capabilities by the WG meeting! Is it critical to get a PR merged to main for this to happen? I'm trying to be aware of demands on everyone's time, especially @ekluzek .

Meanwhile @TeaganKing is working on #1904. I wonder if it's worth having a focused discussion on the best way to implement these more diverse functionalities into the NEON workflow? Would our Thursday CLM meeting be too soon to start discussing this?

@adrifoster
Copy link
Collaborator Author

+1, it would be great to illustrate these capabilities by the WG meeting! Is it critical to get a PR merged to main for this to happen? I'm trying to be aware of demands on everyone's time, especially @ekluzek .

If we want other people to be able to run FATES NEON then yes, otherwise we would have to reference my code or tell them what files to modify - which we can do now

@wwieder
Copy link
Contributor

wwieder commented Jan 10, 2023

sorry, maybe I wasn't clear with my question. Yes, we want this on main, but does it need to come to main before the WG meeting?

On the topic of "hacky user-mods stuff" is there a simple way of using run_neon to handle some of this? How are you actually defining this as a FATES case? It seems like the only new stuff in usermods is related to default/user_nl_clm, is there some way to use the compset we're using to key off what defaults to add? (maybe you and Erik have already discussed this?)

@ekluzek ekluzek added the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Jan 11, 2023
@adrifoster
Copy link
Collaborator Author

adrifoster commented Jan 11, 2023

sorry, maybe I wasn't clear with my question. Yes, we want this on main, but does it need to come to main before the WG meeting?

I guess we would want it on main before the WG meeting if we want to showcase it and then have folks be able to go do it themselves, which I think would be good.

On the topic of "hacky user-mods stuff" is there a simple way of using run_neon to handle some of this? How are you actually defining this as a FATES case? It seems like the only new stuff in usermods is related to default/user_nl_clm, is there some way to use the compset we're using to key off what defaults to add? (maybe you and Erik have already discussed this

The main issue is just that we have multiple folders of user_mods, which works fine but just requires us to update things in two places if updates need to occur (for example with PR #1933). From the user's perspective it will work (the way the PR is right now) the same as a big-leaf method. Just set user_mods to the path to the folders I created. I haven't updated run_neon but that wouldn't be too difficult and we can do that eventually. Right now to run a FATES NEON case it would be similar to creating any other CTSM job but using a FATES compset and referencing the user_mods for the FATES NEON folder:

NEON_SITE=ABBY

./create_newcase --case ${NEON_SITE}_test --res CLM_USRDAT --compset 2000_DATM%1PT_CLM50%FATES_SICE_SOCN_SROF_SGLC_SWAV --run-unsupported --user-mods-dir /glade/work/afoster/CTSM/cime_config/usermods_dirs/NEON/FATES/${NEON_SITE}

We can add this functionality to run_neon as well but I think @ekluzek was thinking this could happen at a later date. Right now the folks who are asking about FATES and NEON are regular FATES users so would not use run_neon. @jkshuman correct?

only needed for ONAQ, but done for all sites for consistency
@wwieder
Copy link
Contributor

wwieder commented Jan 11, 2023

OK, maybe we can combine this with #1933, which would require duplicate changes to usermods_dir/NEON/ONAQ and also updating the 16 PFT surface datasets for the site. With improvements to the overall infrastructure to be done at a later date.

We can discuss at the SE meeting on Thurs.

@jkshuman
Copy link
Contributor

@adrifoster I agree that having it on main before meeting is the best and cleanest way to showcase this. It is on main and not in a user directory. I also agree that FATES users will not be using run_neon to start. run_neon would be a potential future development. We can discuss further on Thursday.

@billsacks
Copy link
Member

Can we somehow modify the way include_user_mods is read to allow for variables?

Interesting. I like the general idea. I can't quite picture how your specific suggestion would work (not necessarily an issue with your suggestion - I may just need you to explain it to me in more detail). One thing to note is that, as currently implemented, the include directories are built up recursively before the shell commands are parsed - see https://github.com/ESMCI/cime/blob/master/CIME/user_mod_support.py (called from the apply_user_mods function in https://github.com/ESMCI/cime/blob/master/CIME/case/case.py). One way I could see this working is to introduce a new env_case xml variable like "include_user_mods_var"; this could be set in config_component.xml based on the compset, and I think apply_user_mods would have access to this.

But I wonder if it's also worth considering some alternative ways to get at this. Specifically, it's now possible to specify multiple --user-mods-dirs on the create_newcase command line (and also to specify a user_mods_dir that is set automatically via the compset, though I'm not sure if that would be useful here). Would it make sense, rather than using include_user_mods, to specify two user-mods-dirs up-front – one for the specific NEON site and then the appropriate "default" based on whether this is a FATES or non-FATES case? I haven't thought this through carefully, so this may not work, but I wanted to put this possibility out there so you could judge whether it could work.

@billsacks
Copy link
Member

But I wonder if it's also worth considering some alternative ways to get at this. Specifically, it's now possible to specify multiple --user-mods-dirs on the create_newcase command line (and also to specify a user_mods_dir that is set automatically via the compset, though I'm not sure if that would be useful here). Would it make sense, rather than using include_user_mods, to specify two user-mods-dirs up-front – one for the specific NEON site and then the appropriate "default" based on whether this is a FATES or non-FATES case? I haven't thought this through carefully, so this may not work, but I wanted to put this possibility out there so you could judge whether it could work.

Oh, given @jkshuman 's comment that run_neon won't be used, this could potentially be a pain for the user. We might have to talk this through so I can understand the workflow better.

@adrifoster
Copy link
Collaborator Author

Oh, given @jkshuman 's comment that run_neon won't be used, this could potentially be a pain for the user. We might have to talk this through so I can understand the workflow better.

Hmm yeah let's discuss Thursday? I like your xml variable suggestion though, that was sort of my line of thinking as well. We just need to be able to get the user_mods functions to have access to it.

additional lat-lon changes to BLAN, UNDE, ORNL
@jedwards4b
Copy link
Contributor

I'm coming to this discussion late but it sounds like you have stuff in usermods_dirs/defaults that you want to apply only when its not a fates case - is that correct? If so can you just change the include_user_mods in the non fates cases to
../defaults.nofates and in that directory have another include_user_mods pointing to defaults (which now contains only what you need for fates)

Copy link
Contributor

@jkshuman jkshuman left a comment

Choose a reason for hiding this comment

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

This looks great @adrifoster Thank you for bringing all this together and documenting it all so clearly.

@adrifoster
Copy link
Collaborator Author

Testing on izumi and cheyenne finished (with some new expected fails):

Cheyenne:
aux_clm and fates had 1 new SHAREDLIB_BUILD fail for ERP_P144x2_Ld30.f45_f45_mg37.I2000Clm50FatesRs.cheyenne_intel.clm-mimicsFatesCold due to the check I added in this PR for not allowing -fire_emis and use_fates (see #1045). I added this test to the expected fails until we fix this issue.

Izumi:
aux_clm had a few fails that were expected due to this PR: The new NEON-FATES-NIWO test was created in this PR so doesn't have a baseline to compare to. And the clm-NEON-NIWO uses a new surface dataset created in @wwieder's PR #1933

    FAIL SMS_D_Mmpi-serial.CLM_USRDAT.I1PtClm51Bgc.izumi_nag.clm-default--clm-NEON-NIWO NLCOMP
    FAIL SMS_D_Mmpi-serial.CLM_USRDAT.I1PtClm51Bgc.izumi_nag.clm-default--clm-NEON-NIWO BASELINE ctsm5.1.dev116: DIFF
    FAIL SMS_D_Mmpi-serial.CLM_USRDAT.I1PtClm51Fates.izumi_nag.clm-Fates--clm-NEON-FATES-NIWO NLCOMP
    FAIL SMS_D_Mmpi-serial.CLM_USRDAT.I1PtClm51Fates.izumi_nag.clm-Fates--clm-NEON-FATES-NIWO BASELINE ctsm5.1.dev116: ERROR BFAIL baseline directory '/fs/cgd/csm/ccsm_baselines/ctsm5.1.dev116/SMS_D_Mmpi-serial.CLM_USRDAT.I1PtClm51Fates.izumi_nag.clm-Fates--clm-NEON-FATES-NIWO' does not exist

fates had the same failure for the baselines for the new fates NEON-NIWO test and then also had a timeout RUN failure for ERS_D_Ld30.f45_f45_mg37.I2000Clm50FatesCruRsGs.izumi_nag.clm-FatesColdPRT2. @glemieux and I tried to figure this one out but in the interest of getting this PR in I added to the expected fails list for now and created a fates-side issue for it.

If we are okay with these test outcomes I think this PR is ready to go. @ekluzek?

Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

I do have a couple small suggestions. But putting approve on this so as to not slow you down. This is a very nice feature addition that is so good to have in place.

We do have the caveat that we duplicated the NEON user-mods for FATES and we do want to change how that's done for maintainability. You should create an issue for that.

This is something that's my fault, but I made it so that 1x1_brazil is just 78PFT in ctsm5.1.dev116. Both mine and your testing seems to indicate that this is OK provided the simpler modes aren't used. I'm actually not sure why this even works as I thought it wouldn't. I'll create an issue on that one as it's my fault. I created issue #1948 for this one.

  • Create an issue about the duplication issue

tools/site_and_regional/subset_data Outdated Show resolved Hide resolved
@ekluzek
Copy link
Collaborator

ekluzek commented Feb 1, 2023

@adrifoster you should update the top description, since the argument names changed. It's useful to look back at closed PR's, and I usually just look at the description. So it is nice to have the description be accurate. That would help future me...

@adrifoster
Copy link
Collaborator Author

We do have the caveat that we duplicated the NEON user-mods for FATES and we do want to change how that's done for maintainability. You should create an issue for that.

Issue created here: #1949

@ekluzek ekluzek merged commit 2291307 into ESCOMP:master Feb 2, 2023
@adrifoster adrifoster deleted the NEON_fates branch May 10, 2023 19:52
@samsrabin samsrabin added the science Enhancement to or bug impacting science label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new capability or improved behavior of existing capability science Enhancement to or bug impacting science
Projects
Status: Done (non release/external)
Development

Successfully merging this pull request may close these issues.

7 participants