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

Add icesheet to compset and support generation of alternative/multiple cism.config files #52

Merged
merged 24 commits into from
Mar 2, 2021

Conversation

billsacks
Copy link
Member

@billsacks billsacks commented Dec 29, 2020

This PR takes two significant steps towards the support of alternative and multiple ice sheets:

(1) It adds an ice sheet designation in the compset name. Now Greenland compsets will have CISM2%EVOLVE%GRIS, Antarctica CISM2%EVOLVE%AIS (not yet functional, but I have added this to the compset definition file for illustrative purposes and so that I could do some initial testing) and compsets with both icesheets will have CISM2%EVOLVE%AIS%GRIS. (NOEVOLVE is also an option for all of those.) I have done this in a backwards-compatible way so that compsets without %AIS or %GRIS (as are still defined in CTSM, CAM and CESM) imply %GRIS. (We can remove this backwards compatibility once all of the CESM components have switched to the new, explicit compset definition.) As discussed in #39, I have changed T1850G to T1850Gg.

(2) It reworks buildnml to support the generation of multiple cism.config files, each with their own default values for some config items. The generated cism.config files now have a suffix designating the ice sheet for which they apply, so we have cism.config.gris and/or cism.config.ais. For now, anything in user_nl_cism applies to all ice sheets, but eventually we'll add a way to make settings for just one ice sheet (see #50).

(There is also a small change in the mct cap that is unrelated to the rest of this PR: it is something that @whlipscomb and I discussed about a month ago.)

@whlipscomb - There's no need for you to review the diffs here, but I'd like to hear your approval – or suggestions for changes – based on the above description of changes.

@Katetc - In addition to giving your okay (or suggestions for changes) based on the above description, files that are most worth looking at are config_component.xml, config_compsets.xml and namelist_definition_cism.xml.

I am currently running aux_glc testing to ensure that this is bit-for-bit with master, and that namelists (both cism_in and cism.config, which is now cism.config.gris) are identical except for expected differences.

Resolves #39

This isn't quite right yet: for example, I know we need separate nmlgen
objects for each ice sheet, and I think we want just a single cism_in
file.
This will give us reliable, repeatable ordering
This makes the file more consistently top-down
This will be important when we have multiple cism.config files but only
a single cism_in.
Where ICESHEET is the given icesheet (gris, ais, etc.).
We'll want all of these defaults to differ for Antarctica, etc.
With the previous code, there was garbage in the param file name in the
ConfigRead call in glad_initialize_instance: it started correct, but
then was filled with garbage after the real text.
To avoid errors with the user_nl_cism file, nmlgen needs to contain all
of the groups from both cism_in and cism.config. However, for the sake
of the cism_in file, we can avoid doing the work of adding the defaults
for the cism.config groups (and vice versa).
Now that there is just one such file, the documentation can go back in
there.
This is needed for @whlipscomb's performance enhancements that leave
some grid cells out of the decomposition. We'll need similar changes in
the NUOPC cap.
@billsacks billsacks self-assigned this Dec 29, 2020
@billsacks
Copy link
Member Author

I have added some minimal documentation changes so that the current documentation is correct (changing T1850G to T1850Gg). Soon we'll need to add more general documentation on multiple ice sheets, but not yet. I have run the full aux_glc test suite; everything is bit-for-bit, with NLCOMP differences just as expected.

@Katetc I'll wait for your go-ahead. It doesn't matter to me whether this comes in before or after the tag you're working on.

@whlipscomb
Copy link
Contributor

@billsacks - I haven't looked at the changes in detail, but everything you say in the summary sounds good to me. I like that it's logical and not too complicated, while preserving backward compatibility.

later). You can run with either existing forcing data (see :numref:`t-with-existing-data`)
or with your own forcing data (see :numref:`t-with-your-own-data`).
compsets have names like T1850Gg, where the final g indicates Greenland.) This compset
uses the active ice sheet model forced by a data land model; all other components are
Copy link
Contributor

Choose a reason for hiding this comment

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

"This compset uses the active ice sheet model for the specified ice sheet(s); all other components...."

Copy link
Member Author

Choose a reason for hiding this comment

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

@Katetc I have added your suggestion, but also kept "forced by a data land model" (were you suggesting that I should get rid of that?)

@Katetc
Copy link
Contributor

Katetc commented Jan 6, 2021

Hi Bill, I have a few issues/questions about the current compset suggestions you made. I think the proposed method is a bit confusing. It seems that any ice sheets not specifically mentioned in the compset are by default, set to NOEVOLVE. But, that makes compsets like CISM2%NOEVOLVE%AIS confusing and redundant. What does that compset imply about the GRIS? Especially since at least one ice sheet must be specified, in the case where all ice sheets are NOEVOLVE it seems strange to have to specify one to specifically be not evolving. And that may also (falsely) indicate to users that something different (ala virtual elevation classes) is happening in one non evolving ice sheet and not the other.

I'm not great at regular expressions, but can we have "CISM2%NOEVOLVE" be the only NOEVOLVE supported compset and then EVOLVE require at least one ice sheet modifier? I feel like this would be more inline with usage.

Also, yes, right now I think this should come in after my CISM_development tag, but keep testing for b4b against the current baselines. You will probably have to do that again after CISM_development brings in new baselines, unfortunately.

I also added one small suggestion for the documentation along the above lines.

@whlipscomb
Copy link
Contributor

@Katetc, thanks for pointing out this possible source of confusion. @billsacks, what do you think? My understanding is that we'll be able to run each ice sheet in either evolve mode or no-evolve mode. So there will be four ways to run if both the AIS and GRIS grids are present. In addition, we'll have several compsets in which one of these grids is present but not both. I haven't quite gotten my head around how each one would be named, so it would help me to see a list of each compset and the associated name (or names, since in some cases we'll need more than one name for backward compatibility).

@billsacks
Copy link
Member Author

@Katetc and @whlipscomb thanks a lot for looking at this. Let me try to clarify: Moving forward (i.e., forgetting about the backwards compatibility bit for now), putting a given ice sheet in the compset name will mean that that ice sheet is included in the model in some form (either evolve or noevolve). If the ice sheet name does not appear in the compset name, that means that ice sheet is not included in any form. So CISM2%NOEVOLVE%AIS means that AIS is included but non-evolving, and CISM doesn't do anything with Greenland: there isn't any Greenland grid in the system. So CISM2%NOEVOLVE%GRIS means something different from CISM2%NOEVOLVE%AIS.

@whlipscomb 's question about having one ice sheet in evolve mode and one noevolve is a good one. I wasn't really sure how to handle this. The issue is that, currently, there is some logic in nuopc that assumes that ice evolution is on or off for all of GLC – so doesn't allow for the possibility that one ice sheet is evolving and the other is not. (I forget the details of this.) So I created the compset names with that requirement in mind. I realize that, long-term, there may be a need for making this more flexible.

I'll be honest: I haven't given a lot of thought to how we want this to work long-term. I'm happy to either have that conversation soon (like next week) or just get something working now and then revisit this later. One long-term possibility that would let us keep the compsets simpler would be to keep the compset specification like I have here (so that, at the compset level, you just specify evolution on or off for glc as a whole), but then have this set separate xml variables like CISM_EVOLVE_GREENLAND and CISM_EVOLVE_ANTARCTICA. After creating the case, the user could change those individual variables. That could make sense if the vast majority of usages would have ice evolution either on for all ice sheets or off for all ice sheets, and that doing a mix of the two would be less common. I'm not sure if that's the case or not.

If, on the other hand, you think it's common that people will want to have ice evolution on for one ice sheet but off for another, such that we'd want to enable that possibility at the compset level, then this would take some rework. In that case, we have two choices: (1) Set up the compset long names now to facilitate that long-term plan, but for now check to ensure that all ice sheets agree about whether they're evolving or not (as required by current nuopc code); or (2) For now, set up the compset long names to fit in with what nuopc allows, and then change them later. If we went with (2), we wouldn't necessarily need to change the compset aliases, just the long names. I am open to either, but off-hand I'm having trouble seeing exactly how to implement things to allow separate specification of ice evolution for the different ice sheets. This is because nuopc needs an overall xml variable CISM_EVOLVE and there isn't a straightforward way that I can think of to specify that variable if the compset separately specifies ice evolution for the different ice sheets.

This is all to say: my preference would be to set things up for now to agree with what nuopc allows / requires, and then change things later when someone has time to add more flexibility to nuopc. Please let me know your thoughts.

@billsacks
Copy link
Member Author

Notes from discussion with @whlipscomb and @Katetc

It is important in the not-too-distant future to be able to have one ice sheet evolving and the other not. It might be that this works right for NUOPC as long as CISM_EVOLVE is set to true in this case – i.e., things might just work if CMEPS sees GLC as evolving, but in fact one of the ice sheets is not evolving – but this will need some testing. In any case, though, we want the compset naming to support this possibility.

So I'm going to rework the compset naming to have options like GIS-EVOLVE / GIS-NOEVOLVE and similarly for AIS. Then, if there is any occurrence of -EVOLVE, we will set the overall CISM_EVOLVE to TRUE; otherwise CISM-EVOLVE will be FALSE.

So we can have compsets like this:

  • CISM2%GIS-EVOLVE

  • CISM2%GIS-NOEVOLVE

  • CISM2%AIS-EVOLVE%GIS-NOEVOLVE

@billsacks
Copy link
Member Author

I'm finally getting back to this. I realized one issue with the plan we came up with is that it's hard (or impossible) to prevent someone from adding multiple conflicting options to the compset long name: CISM2%GRIS-EVOLVE%GRIS-NOEVOLVE. I can't see a good way to prevent this, so if we want this scheme, we'll need to just leave the burden on the user to not do something like this. (If they do, it will be indeterminate whether GRIS ends up evolving.)

@gunterl
Copy link
Contributor

gunterl commented Feb 25, 2021 via email

@billsacks
Copy link
Member Author

billsacks commented Feb 25, 2021

Is there a way to check the number of occurrences of GRIS or AIS in the compset name?

In principle, yes. But we generally try to avoid this kind of special-purpose parsing of compset names by components, and I feel like it's the kind of thing that could easily cause more problems than it solves.

@whlipscomb
Copy link
Contributor

@billsacks, I'm OK with leaving it up to the user to avoid logically impossible compsets.

Kate Thayer-Calder and Bill Lipscomb suggested this change in compset
specification to allow specifying ice evolution separately for each ice
sheet (for now they all must agree, but we will relax that requirement
in the future).

So now we can have compsets like this:

CISM2%GIS-EVOLVE

CISM2%GIS-NOEVOLVE

(eventually)
CISM2%AIS-EVOLVE%GIS-NOEVOLVE
@billsacks
Copy link
Member Author

@whlipscomb @Katetc FYI, I have reworked the compset specification as we discussed.

@whlipscomb
Copy link
Contributor

@billsacks, Thanks!

Copy link
Contributor

@Katetc Katetc left a comment

Choose a reason for hiding this comment

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

Looks good!

Put the '.config' at the end, so that tools that try to auto-detect the
file format can guess correctly. So use files like 'cism.gris.config'
rather than 'cism.config.gris'.
I think this was broken starting in cism2_1_75.

Resolves #55
@billsacks billsacks merged commit b2e754f into master Mar 2, 2021
@billsacks billsacks deleted the icesheet_in_compset branch March 2, 2021 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an explicit "Greenland" designation in the compset name
4 participants