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

Removing CLM from namelist variables #3780

Closed
bishtgautam opened this issue Nov 17, 2020 · 21 comments
Closed

Removing CLM from namelist variables #3780

bishtgautam opened this issue Nov 17, 2020 · 21 comments

Comments

@bishtgautam
Copy link
Contributor

bishtgautam commented Nov 17, 2020

Many namelist variables that are shared by CTSM and E3SM include CLM

https://gist.github.com/bishtgautam/f912aaa94f4c95c28ecc071864902281

cc: @billsacks

@billsacks
Copy link
Member

@bishtgautam I agree with the principle of removing "CLM" from these names, but I'm concerned about the possible problems caused by doing so. At the very least, this would require extensive testing in CESM as well as E3SM to ensure that nothing is broken. It looks like the vast majority of the issues are in the data models, and this would become easier for you when we split the e3sm and cesm data model code, though the timeframe for doing that is uncertain right now. What is the priority of this for you, and are there perhaps a few higher-priority ones that you could do first, without doing all of them?

@mvertens @ekluzek

@bishtgautam
Copy link
Contributor Author

In E3SM, we were hoping to replace most of namelist options with CLM to ELM by v2.0 release. @rljacob can provide an estimate of E3SM v2.0 release deadline.

are there perhaps a few higher-priority ones that you could do first

This sounds reasonable and let me come up with a list.

@rljacob
Copy link
Member

rljacob commented Nov 17, 2020

Actually I can't because it depends on the progress of v2 tuning, v2 sims and new non-BFB development. We should have our own versions of the data models and coupler by then so may not need a uniform solution but it would be good to find one anyway because the data models should be agnostic about the names of other models.

@ekluzek
Copy link
Contributor

ekluzek commented Nov 17, 2020

It does make sense that data models should be agnostic toward names of other models. And I do see the point of changing these. I agree with @billsacks that I'm concerned about the practicality of doing this. I think the options that are under the component cime_config are able to be truly independent, but these data model XML/namelist things are shared by both. So we should make sure the new names are independent of model. I wonder if you should just remove the "C" in the name so it has "LM" for Land Model"? CESM has also moved to CTSM now, so CLM doesn't really fit the CESM world either. So what if we just come up with another word for the "C" so it doesn't seem as objectionable?

I think you can replace the things under ELM/cime_config with ELM_ without much of a problem. It's these shared things under cime data components that are a problem. And it doesn't seem like they (the things pointed out here under the data models) should be replaced with ELM for E3SM and CTSM for CESM -- because that would duplicate them for both.

@mvertens
Copy link
Contributor

mvertens commented Nov 17, 2020 via email

@rljacob
Copy link
Member

rljacob commented Nov 17, 2020

That's a way around this but it doesn't tackle the root problem that, for example, datm config files should not be filled with variables that have "CLM" in them even if its a CESM version of datm.

@bishtgautam
Copy link
Contributor Author

I agree with everyone that it would be a long term goal to separate out data models and renaming all variables to remove CLM will require a lot of testing. Can we consider the following four namelist variables for renaming?

DIN_LOC_ROOT_CLMFORC
DATM_CLMNCEP_YR_ALIGN
DATM_CLMNCEP_YR_START
DATM_CLMNCEP_YR_END

@billsacks
Copy link
Member

Hmmmm, I can see how those would be good things to change, and yet my gut reaction is that those are also ones that might cause some of the most disruption – requiring testing, maybe changes to CESM/CTSM, documentation, and potentially many users' personal scripts as well as machine ports that specify DIN_LOC_ROOT_CLMFORC. I'm not sure what the best way is to proceed here, because I agree that these should be changed long-term, so I guess it is just a question of when it can reasonably fit into the priority list, given that it will require time on the CESM side as well as the E3SM side.

Is there any chance that it's possible to introduce new variables that serve these purposes for e3sm? I could imagine this maybe working if you use different datm modes in e3sm vs. cesm anyway, in which case you may be able to use something different from these CLMNCEP variables for your datm modes.

Otherwise, this gets me thinking about how it would be really nice to have a capability in cime to facilitate renaming xml variables in a backwards compatible way - for this and other purposes. Among other things, this would let you (for example) rename DATM_CLMNCEP_YR_ALIGN to DATM_YR_ALIGN within cime, but still do ./xmlquery DATM_CLMNCEP_YR_ALIGN or ./xmlchange DATM_CLMNCEP_YR_ALIGN=XXX (to support old users' scripts) and those would still work, perhaps with a deprecation warning. I could imagine implementing this by allowing xml variables to have an optional list of alternate names – so you'd change the main name, but list the old name in the list of alternate names. I have no idea how hard this would be; I suspect it wouldn't be trivial, but it could be a valuable feature long-term as backwards compatibility becomes more and more important with cime.

@billsacks
Copy link
Member

@mvertens @ekluzek I'm interested in whether you share my gut reaction or if you think it wouldn't be too bad to rename these. (This really was just a gut reaction rather than being based on any analysis of code or documentation to see what would need to change.)

@bishtgautam
Copy link
Contributor Author

Is there any chance that it's possible to introduce new variables that serve these purposes for e3sm?

Let me give that this a try and I will report back.

./xmlquery DATM_CLMNCEP_YR_ALIGN or ./xmlchange DATM_CLMNCEP_YR_ALIGN=XXX (to support old users' scripts) and those would still work, perhaps with a deprecation warning.

IMO, CIME should give an error message and tell users to instead use DATM_YR_ALIGN. Otherwise, users will continue to use DATM_CLMNCEP_YR_ALIGN.

@billsacks
Copy link
Member

The problem is that an error message doesn't allow for any time of backwards compatibility. Without backwards compatibility we are in the situation of not allowing any changes until everyone is ready to change all at once, which can be a challenge.

@rljacob
Copy link
Member

rljacob commented Nov 18, 2020

People will change their scripts slowly as they update to whatever version of CESM will have this. No need for everyone to change at once.

@ekluzek
Copy link
Contributor

ekluzek commented Nov 18, 2020

It looks like changing those wouldn't be. maybe as hard as we originally thought. Most of the instances of this are just in cime, with some others in CTSM (and obviously in. ELM as well). But, there's also a bunch of instances in CDEPS. Those would need to change as well.

I see one testmod with it for CTSM. PTCLM has a bunch of uses that would need to change. The documentation would need to change. And there are some unsupported contrib scripts. I don't see changes in CESM, mosart, or rtm. I don't think you'd see it outside of those components.

So I think it would be OK to change for CTSM. with just the one test needing to change immediately. The other CTSM changes as they are in tools. and documentation so it could be OK to do them later. But, I'm concerned about CDEPS as it's more extensive and needs to be coordinated with cime. So that means the nuopc testing would. need to be redone.

@bishtgautam
Copy link
Contributor Author

I tried defining a new variable just for E3SM, but it didn't work.

@jgfouca
Copy link
Contributor

jgfouca commented Feb 24, 2021

@bishtgautam , can you give us an update on this issue? Are CIME changes needed?

@bishtgautam
Copy link
Contributor Author

The changes required to fix this are not needed in CIME itself but needed in data models. I have not spent time fixing it since Nov and this is now not a high-priority item for me.

@github-actions
Copy link
Contributor

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@ekluzek
Copy link
Contributor

ekluzek commented May 18, 2023

I think this would still be useful to do. Albeit not something high priority.

As noted above, some of these changes are really in the data models and now outside of CIME. The main variables within CIME are:

DIN_LOC_ROOT_CLMFORC
CLM_BLDNML_OPTS
USE_SHARED_CLM
CLM_USRDAT_NAME
CLM_USE_PETSC

CLM_BLDNML_OPTS is just in an example in the mapping tools so doesn't matter.
USE_SHARED_CLM is wholly within the Makefile

CLM_USE_PETSC isn't used by CTSM in CESM, and is only used to set USE_PETSC in the build which is a little strange.

We have another issue to deal with DIN_LOC_ROOT_CLMFORC #3097

I've been thinking that CLM_USRDAT_NAME should be generalized to something like LND_USERGRID_NAME, but that will require some coordination across repositories. I'll likely make an issue for that at some point.

Outside of those this is what I see..

git grep CLM | grep -v -e ^ChangeLog: -e ^doc/ | grep -v -e DIN_LOC_ROOT_CLMFORC -e CLM_BLDNML_OPTS -e USE_SHARED_CLM -e CLM_USRDAT_NAME -e CLM_USE_PETSC -e '>>> '
CIME/SystemTests/README:SSP    smoke CLM spinup test (only valid for CLM compsets with CN or BGC)  (TODO - change to SPL)
CIME/SystemTests/README:       do an initial spin test (setting CLM_BLDNML_OTPS to -bgc_spinup_on)
CIME/SystemTests/README:LII    CLM initial condition interpolation test
CIME/case/case.py:        ('1850_CAM60_CLM50%BGC-CROP_CICE_POP2%ECO%ABIO-DIC_MOSART_CISM2%NOEVOLVE_WW3_SESP_BGC%BDRD', ['1850', 'CAM60', 'CLM50%BGC-CROP', 'CICE', 'POP2%ECO%ABIO-DIC', 'MOSART', 'CISM2%NOEVOLVE', 'WW3', 'SESP'])
CIME/case/case.py:        ('1850_CAM60_CLM50%BGC-CROP_CICE_POP2%ECO%ABIO-DIC_MOSART_CISM2%NOEVOLVE_WW3_SIAC_SESP_BGC%BDRD_TEST', ['1850', 'CAM60', 'CLM50%BGC-CROP', 'CICE', 'POP2%ECO%ABIO-DIC', 'MOSART', 'CISM2%NOEVOLVE', 'WW3', 'SIAC', 'SESP'])
Binary file CIME/non_py/cprnc/test_inputs/clm2.h0.subset.control.nc matches
Binary file CIME/non_py/cprnc/test_inputs/clm2.h0.subset.test.nc matches
Binary file CIME/non_py/cprnc/test_inputs/clm2.h1.subset.control.nc matches
Binary file CIME/non_py/cprnc/test_inputs/clm2.h1.subset.test.nc matches
tools/mapping/examples/wrf_clm:to create a new regional grid for running WRF-CLM within the CESM
tools/mapping/gen_mapping_files/gen_ESMF_mapping_file/README:   Use the CLM naming convention
tools/mapping/gen_mapping_files/gen_ESMF_mapping_file/create_ESMF_map.sh:  echo '   Use the CLM naming convention'
tools/mapping/gen_mapping_files/gen_ESMF_mapping_file/create_ESMF_map.sh:CLMNAME="FALSE"
tools/mapping/gen_mapping_files/gen_ESMF_mapping_file/create_ESMF_map.sh:      CLMNAME="TRUE"
tools/mapping/gen_mapping_files/gen_ESMF_mapping_file/create_ESMF_map.sh:if [ $CLMNAME == "TRUE" ]; then

The README references could be removed as those have been moved to CTSM long ago. The create_ESMF_map.sh changes the output mapping filenames. But, with CESM having moved to NUOPC, I don't think this even matters anymore?

@klindsay28
Copy link
Collaborator

CLM_BLDNML_OPTS is referred to in a testmods_dir file of MOM_interface.
I'm not certain why it is used. Perhaps @alperaltuntas knows.

This highlights the original comment by @billsacks about needing to test such a change extensively.

@github-actions
Copy link
Contributor

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jun 18, 2023
@github-actions
Copy link
Contributor

This issue was closed because it has been stalled for 5 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants