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

Fixes F compsets to support ne4np4 #3333

Merged
merged 2 commits into from
Feb 7, 2020

Conversation

bishtgautam
Copy link
Contributor

@bishtgautam bishtgautam commented Dec 3, 2019

Fixes F1850SC5-CMIP6, F2010C5-CMIP6-HR and F2010C5-CMIP6-LR.
Moves the definition of fsurdat and finidat from use_cases namelist to
land model default namelist

[BFB]

@bishtgautam bishtgautam added Land maint-1.0 PRs for or issues about maint-1.0 labels Dec 3, 2019
@tangq
Copy link
Contributor

tangq commented Dec 3, 2019

Have you tested whether the F2010C5-CMIP6-LR (maint-1.0) and F2010SC5-CMIP6 (master) compsets pick up the same land files before and after this change? Do you plan to make consistent changes on master?

@bishtgautam
Copy link
Contributor Author

bishtgautam commented Dec 3, 2019

In E3SM-v1.0, we made the mistake when defining new compsets using xml files under components/clm/bld/namelist_files/use_cases by including grid specific values (i.e. fsurdat, finidat). All grid specific entries should go in components/clm/bld/namelist_files/namelist_defaults_clm4_5.xml.

Due to this mistake, as noted on this confluence page, ./create_test SMS_D.ne4_ne4.F2010C5-CMIP6-LR has finidat and fsurdat that corresponds to ne30np4 even though the --res of the test is ne4np4.

I have verified that this PR now corrects the finidat and fsurdat for ./create_test SMS_D.ne4_ne4.F2010C5-CMIP6-LR. The test ran successfully was able to take few time steps after changing ./xmlchange PIO_TYPENAME=netcdf, which is also noted on the confluence page, but the test crashed in the atmosphere model.

@tangq, I can fix all other incorrectly defined compsets for maint-1.0 in this PR instead of just fixing F2010C5-CMIP6-LR. Would you prefer that? What are the various --res values that you would want to make sure aren't broken by this PR.

A separate PR is needed to fix master.

(cc: @cameronsmith1)

@cameronsmith1
Copy link
Contributor

The hgrid selection flags can also be done in the use-case file. Is there a reason for transferring it to the defaults file? It seems there is a risk of changing previous compsets (some of which may be very old, and which we may not care about).

I do acknowledge that the way I/we have been creating specific compsets to avoid the possibility of altering earlier compsets also causes reduction in flexibility for new versions of the model. I still think there should be an effort to figure out a better policy for managing compsets for the long term.

@tangq
Copy link
Contributor

tangq commented Dec 3, 2019

@bishtgautam , I have 2 meetings this afternoon. I will get back to you later today or tomorrow.

@tangq
Copy link
Contributor

tangq commented Dec 4, 2019

@bishtgautam, on maint-1.0, the affected compsets are F1850SC5-CMIP6, F2010C5-CMIP6-LR, and F2010C5-CMIP6-HR. These compsets were created based on the existing CMIP6 compsets at that time, so the land files are set in the use_case files. The F1850SC5-CMIP6 and F2010C5-CMIP6-LR compsets were created for the ne30np4 resolution, while F2010C5-CMIP6-HR for the ne120np4 resolution.

On master, it is the F2010SC5-CMIP6 compset on the ne30np4 grid.

It would be great if you can fix all the affected compsets on maint-1.0 on this PR and a separate PR for the master. I am fine with either way where the land settings are set. After your changes, just make sure the files are picked up as they are used to be. Thanks.

@cameronsmith1
Copy link
Contributor

@bishtgautam : will this affect the B compsets too?

When you put this on master, there will also be the BGC compsets to fix, although they may not have been merged from maint-1.1 to master yet.

Moves the definition of fsurdat and finidat from use_cases namelist to
land model default namelist

[BFB]
@bishtgautam
Copy link
Contributor Author

@cameronsmith1 Which B compsets + res should I test to make sure these changes don't adversely impact them?

It would be easier to fix master directly than trying to merge maint-1.0 or maint-1.1 to master. Also, I'm wasn't planning on fixing maint-1.1, unless someone has a need to fix it.

@bishtgautam bishtgautam force-pushed the bishtgautam/lnd/F2010C5-CMIP6-LR branch from 6901e26 to e951011 Compare December 4, 2019 16:49
@bishtgautam bishtgautam changed the title Fixes F2010C5-CMIP6-LR for ne4np4 Fixes F compsets to support ne4np4 Dec 4, 2019
@bishtgautam
Copy link
Contributor Author

@tangq This PR now fixes F1850SC5-CMIP6, F2010C5-CMIP6-LR, and F2010C5-CMIP6-HR compsets.

@@ -18,7 +18,6 @@
<stream_year_last_popdens phys="clm4_5" use_cn=".true." ndepsrc="stream" >1955</stream_year_last_popdens>

<!-- CMIP6 HighResMIP compsets -->
<fsurdat hgrid="ne120np4" >lnd/clm2/surfdata_map/surfdata_ne120np4_simyr1950_c20180108.nc </fsurdat>
Copy link
Contributor

Choose a reason for hiding this comment

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

@bishtgautam , why do some compsets still have finidat defined in the use_case xml file? I thought your plan was to move them all to the defaults xml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only made changes for fsurdat + finidat for F1850SC5-CMIP6, F2010C5-CMIP6-LR, and F2010C5-CMIP6-HR. While change for fsurdat is straightforward, the finidat is a little complicated. If you provide me with res+compset for all CMIP6 compsets that are supported in maint-1.0, I will make relevant changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about other CMIP6 F-compsets, but there are what I used on maint-1.0:
F1850SC5-CMIP6.ne30
F2010C5-CMIP6-LR.ne30
F2010C5-CMIP6-HR.ne120

I made the comment above on the 1950 file, but it is also true for 1850_CMIP6_control.xml and 1850_SCMIP6_control.xml (one of which is used by F1850SC5-CMIP6) that contain the finidat setting. Is there a reason why it's not moved to the default file? As long as there is a reason for that (not overlooked), I am fine with the changes.

@cameronsmith1
Copy link
Contributor

@cameronsmith1 Which B compsets + res should I test to make sure these changes don't adversely impact them?

I would say: all of the compsets in cime/config/e3sm/allactive/config_compsets.xml for all of the different resolutions that e3sm has ever used. This will be a huge set. Most combinations have never been used in practice, but I see no easy way to know which have been used and which haven't. Although, there are certainly some that are particularly critical, most notably the CMIP6 configurations (including the hi-resMIP and BGC configurations), IMHO.

The root of the problem is a conflict between two obvious goals:
A) Don't change existing compsets, so they can be rerun with newer code,
B) Allow configurations to run with the latest version of datasets, and allow users to run with new resolution+compset combinations.

So far with E3SM, (A) has been the greater priority.

How about a different strategy to achieve your aims?:

  1. Add 'hgrid=xx' to the fsurdat and finidat lines in the CLM use_case files (it is already there for some of them).
  2. Add the ne4np4 defaults to the CLM defaults.xml file.

I think this should preserve existing configurations because #1 should over-ride the defaults file for those resolutions (correct?), and the ne4np4 resolutions will default as you specify them (there is a risk that any ne4np4 test cases will change, but that will be easy to check by running the test cases).

The other strategy I see is to add the ne4np4 defaults to the relevant CLM use_case files, and add hgrid=xx as needed. This seems slightly lower risk, but the previous option is better for ne4np4 in the future (IMHO), so I would go with the other strategy.

It would be easier to fix master directly than trying to merge maint-1.0 or maint-1.1 to master. Also, I'm wasn't planning on fixing maint-1.1, unless someone has a need to fix it.

At some point maint-1.1 will need to be merged to master and all the compsets should work. Hence, if you put changes into maint-1.0 that will stop the BGC compsets from getting the correct defaults on master after the maint-1.1 merge, it could be a big mess.

@bishtgautam
Copy link
Contributor Author

The root of the problem is a conflict between two obvious goals:
A) Don't change existing compsets, so they can be rerun with newer code,

If you try to achieve the above goal, compsets for v2.0 would need to be given a different alias and we can create these new compsets correctly this time.

Add 'hgrid=xx' to the fsurdat and finidat lines in the CLM use_case files (it is already there for some of them).

Unfortunately, this doesn't work. My suggestion is to only fix compsets on maint-1.0 that we want to run on a resolution that is different from the resolution those compsets were initially designed to run on.

At some point maint-1.1 will need to be merged to master and all the compsets should work. Hence, if you put changes into maint-1.0 that will stop the BGC compsets from getting the correct defaults on master after the maint-1.1 merge, it could be a big mess.

I'm assuming that current watercycle and BGC compsets on maint-1.0 and maint-1.1 won't be exactly BFB in v2.0. If that is true, you can either:

  • Merge maint-1.0 and maint-1.1 to master; then fix the compsets to be consistent, or
  • Directly fix the compsets on master for v2.0.

@@ -271,7 +294,7 @@ lnd/clm2/surfdata_map/surfdata_1x1_brazil_simyr1850_c140610.nc</fsurdat>


<fsurdat hgrid="ne30np4" sim_year="1850" use_crop=".false." >
lnd/clm2/surfdata_map/surfdata_ne30np4_simyr1850_c180306.nc</fsurdat>
lnd/clm2/surfdata_map/surfdata_ne30np4_simyr1850_2015_c171018.nc</fsurdat>
Copy link
Member

Choose a reason for hiding this comment

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

Why is the timestamp going backwards? From 180306 to 171018 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because all supported 1850 compsets were using the older file. (e.g. https://github.com/E3SM-Project/E3SM/pull/3333/files#diff-7ece8b6f482388c76e3a4cab97c792eeL34)

Copy link
Contributor

@cameronsmith1 cameronsmith1 Dec 5, 2019

Choose a reason for hiding this comment

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

The c171018 file is the surfdata file used in the CMIP6 compsets. I don't know where the c180306 file came from, or why it is a default. (I entered this comment before I saw the comment from @bishtgautam )

Copy link
Contributor

Choose a reason for hiding this comment

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

Where did the c180306 file come from?
What was it for?

@cameronsmith1
Copy link
Contributor

The root of the problem is a conflict between two obvious goals:
A) Don't change existing compsets, so they can be rerun with newer code,

If you try to achieve the above goal, compsets for v2.0 would need to be given a different alias and we can create these new compsets correctly this time.

I expect the v2 simulations will use the same CMIP6 input files but change the tuning. If that is true, then the v2 compsets will need different aliases.

What do you consider to be doing them 'correctly'?

Add 'hgrid=xx' to the fsurdat and finidat lines in the CLM use_case files (it is already there for some of them).

Unfortunately, this doesn't work. My suggestion is to only fix compsets on maint-1.0 that we want to run on a resolution that is different from the resolution those compsets were initially designed to run on.

Why won't my idea work?

I think your strategy will ultimately work for the CMIP6 compsets. However, there are others, such as the AV1C compsets (used in the development, initial tuning, and publication of the atm for v1) and the v0 compsets (the baseline from which E3SM improvements have been measured), which don't specify the land datasets, and just use whatever is the land default. I think some of the AV1C compsets were run with ne30np4 (can you remember @tangq ?), so their land files will now be different.

At some point maint-1.1 will need to be merged to master and all the compsets should work. Hence, if you put changes into maint-1.0 that will stop the BGC compsets from getting the correct defaults on master after the maint-1.1 merge, it could be a big mess.

I'm assuming that current watercycle and BGC compsets on maint-1.0 and maint-1.1 won't be exactly BFB in v2.0. If that is true, you can either:

* Merge maint-1.0 and maint-1.1 to master; then fix the compsets to be consistent, or

* Directly fix the compsets on master for v2.0.

The compsets are already not BFB with master because the code has changed. But probably the better question is whether the same namelist files get generated. The answer to this is probably already 'no', but I think that is mostly because new namelist parameters have been added, which shouldn't really count. Hence the even better question is whether any of the compset-relevant namelist options are different. This is less well defined but I think it is what we really mean.

@tangq
Copy link
Contributor

tangq commented Dec 5, 2019

@cameronsmith1 , do you mean these AV1C compsets on ne30?
Screen Shot 2019-12-04 at 5 09 45 PM

@cameronsmith1
Copy link
Contributor

cameronsmith1 commented Dec 5, 2019

Yes. Those AV1C compsets just pickup the default land options (AFAIK). The older AV1C compsets probably don't matter much (IMHO). But I recall that the Rasch E3SM-atm model description paper was based on the AV1C-04P2 compsets. My recollection is those simulations used ne30np4, although I could be wrong.

Other papers may have been based on other AV1C versions.

@bishtgautam
Copy link
Contributor Author

@cameronsmith1

What do you consider to be doing them 'correctly'?

Don't add grid specific values in the xml file when defining a compset

Why won't my idea work?

I tried adding hgrid = ne30np4, but the case for--res ne4_ne4 still picks up the file corresponding to ne30np4. I have no idea if this is a bug related to how the xml file is being correctly parsed or the xml-related code wasn't intended to what we are trying to do.

@cameronsmith1
Copy link
Contributor

What do you consider to be doing them 'correctly'?

Don't add grid specific values in the xml file when defining a compset

As I understand it, this is essentially the policy B from my earlier comment. Unfortunately it will inevitably break policy A (ability to recreate existing compsets in a modern codebase). Ultimately, I think it would be best to setup a hybrid system that follows either policy A or B depending on the type of simulation (A for CMIP simulations, and B for general simulations).

Why won't my idea work?

I tried adding hgrid = ne30np4, but the case for--res ne4_ne4 still picks up the file corresponding to ne30np4. I have no idea if this is a bug related to how the xml file is being correctly parsed or the xml-related code wasn't intended to what we are trying to do.

This has worked when I have tried it, so I am not sure why it isn't working for you. I just talked to @tangq and he confirmed that it worked for him recently for the atm xml file.

@bishtgautam
Copy link
Contributor Author

maint-1.0 has <fsurdat hgrid="ne120np4" > ... </fsurdat> on 2010_CMIP6HR_control.xml#L21. The fsurdat is incorrectly set for the following test using maint-1.0:

>./create_test SMS.ne4_ne4.F2010C5-CMIP6-HR --no-build

>cat <case-dir>/CaseDocs/lnd_in | grep fsur
 fsurdat = '/pic/projects/climate/csmdata//lnd/clm2/surfdata_map/surfdata_ne120np4_simyr2010_c20181023.nc'

Maybe it is only an issue with land xml or the way fsurdat is defined in the xml file. Do you guys have any idea what is going wrong?

@cameronsmith1
Copy link
Contributor

Unfortunately I haven't delved deeply into the xml processing. All I can say is that when I have used the hgrid condition it has worked, although I confess that I haven't tested that it did the right thing for the right reason.

@rljacob do you know what is going on? Or who else might know about the xml processing?

@rljacob
Copy link
Member

rljacob commented Dec 5, 2019

@jgfouca should be able to help.

@jgfouca
Copy link
Member

jgfouca commented Dec 5, 2019

I'm a bit unclear on the problem. I tried this:

%  ./create_test SMS.ne4_ne4.F2010C5-CMIP6-HR --no-build
% cd $CASE
% ./xmlquery GRID
GRID: a%ne4np4_l%ne4np4_oi%ne4np4_r%r05_m%oQU240_g%null_w%null

At first glance, this looks correct. Is there something else I should be testing?

@rljacob
Copy link
Member

rljacob commented Jan 3, 2020

This PR needs a better description. Open an issue for what exactly was broken about the ne4 F compsets. In general, they have been working fine. Clarify title.

@rljacob
Copy link
Member

rljacob commented Jan 27, 2020

@bishtgautam what is the status of this PR?

@rljacob
Copy link
Member

rljacob commented Jan 30, 2020

Go ahead and merge this.

singhbalwinder added a commit that referenced this pull request Feb 7, 2020
Fixes F compsets to support ne4np4

Fixes F1850SC5-CMIP6, F2010C5-CMIP6-HR and F2010C5-CMIP6-LR.
Moves the definition of fsurdat and finidat from use_cases namelist to
land model default namelist

[BFB]

* bishtgautam/lnd/F2010C5-CMIP6-LR:
  Fixes F1850SC5-CMIP6 and F2010C5-CMIP6-HR
  Fixes F2010C5-CMIP6-LR for ne4np4
@singhbalwinder singhbalwinder merged commit e951011 into maint-1.0 Feb 7, 2020
@bishtgautam bishtgautam deleted the bishtgautam/lnd/F2010C5-CMIP6-LR branch October 2, 2020 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Land maint-1.0 PRs for or issues about maint-1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants