ESCOMP / CISM-wrapper

Community Ice Sheet Model wrapper for CESM
http://www.cesm.ucar.edu/models/cesm2.0/land-ice/
Other
3 stars 15 forks source link

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

Closed billsacks closed 3 years ago

billsacks commented 3 years ago

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

billsacks commented 3 years ago

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 commented 3 years ago

@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.

Katetc commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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:

billsacks commented 3 years ago

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 commented 3 years ago

Hi Bill, Is there a way to check the number of occurrences of GRIS or AIS in the compset name? This way if it's greater than 1 we could add an error message. Otherwise it's too bad and the user should be careful. Gunter

On Thu, Feb 25, 2021 at 11:14 AM Bill Sacks notifications@github.com wrote:

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.)

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CISM-wrapper/pull/52#issuecomment-786100759, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB6QV6GZID4T4CRIRDDFBNLTA2HOXANCNFSM4VNPJQJQ .

-- Gunter Leguy, Ph.D (he/him) Project Scientist National Center for Atmospheric Research cell: (575) 418 1021 desk: (303) 497 1790

billsacks commented 3 years ago

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 commented 3 years ago

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

billsacks commented 3 years ago

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

whlipscomb commented 3 years ago

@billsacks, Thanks!