ESCOMP / MOSART

Model for Scale Adaptive River Transport, Mosart, part of the Community Earth System Model
http://www.cesm.ucar.edu/
Other
8 stars 27 forks source link

Pass tdepth as needed for hillslope model #46

Closed ekluzek closed 2 years ago

ekluzek commented 2 years ago

Pass tdepth/tdepth_max as needed for the hilllslope model. Add a trigger for it from CMEPS.

Also don't set direction file if MOSART is turned off. Remove do_rtm namelist trigger and use MOSART_MODE instead.

Fixes #47 Fixes #36 Fixes #45 Fixes #44 Fixes #20

ekluzek commented 2 years ago

@billsacks I'd like you to look at this and let's talk about it this afternoon. The specific change I'm not convinced is right is that I have buildnml change MOSART_MODE under certain conditions. This does work. But, philosophically I think it likely should be done in buildlib rather than buildnml. So I wanted to hear what you thought about that.

billsacks commented 2 years ago

You might not like my answer:

I don't like having buildnml or buildlib change xml values. I feel like this is confusing from both a user and developer perspective. I think it's better to maintain a consistent flow: The compset can set the default value of xml variables, and xml variables can set the default value of namelist variables; other than that, it should be up to the user to set things explicitly. I don't feel like this needs to be followed 100%, but there should be a very good reason for not following it.

In this case, I think I understand the reason for setting MOSART_MODE automatically, so there is one less thing for a user to set when running accelerated spinup. However, in my view, that benefit does not justify violating what I feel should be a general principle in the user interface of CESM's Case Control System.

I can see a few alternatives for how to handle this:

(1) Introduce either compsets or a user mods directory for turning on CLM spinup. A compset would be straightforward here (use SROF), but I'm thinking a user mods directory might be best to avoid a proliferation of compsets. Users can then add the appropriate --user-mods-dir setting when creating a case where they want spinup active. This would set CLM_ACCELERATED_SPINUP along with setting ROF_MODE appropriately (note generic rename: see point (3) for more on this, but it could be renamed even if keeping this variable defined in MOSART: then you don't need separate logic all over the place for MOSART_MODE vs. RTM_MODE, etc.).

(2) If (1) isn't possible, or you want to support manually changing CLM_ACCELERATED_SPINUP after a case is created, then: Instead of setting MOSART_MODE (which again, I'd suggest renaming to ROF_MODE) based on CLM_ACCELERATED_SPINUP, instead leave this up to the user, but issue an error message if they haven't done this. This logic should probably be in CTSM, since it is really CTSM-specific. I'd suggest something like: If CLM_ACCELERATED_SPINUP is on and ROF_MODE is "ACTIVE", issue an error saying that the user should also set ROF_MODE to "NULL". However, also provide an option of ROF_MODE = "ACTIVE_FORCE" that would bypass this check. So the standard value of ROF_MODE would be "ACTIVE", and then if someone turns on CLM_ACCELERATED_SPINUP, then they should either set ROF_MODE to "NULL" or to "ACTIVE_FORCE" depending on what they want. I (significantly) prefer the explicitness of this solution to the current invisible change of MOSART_MODE.

And a tangential point, alluded to above:

(3) Based on where MOSART_MODE is referenced – in CMEPS and not (as far as I can see) in MOSART itself: it really feels to me like there should be a generically-named ROF_MODE variable defined in CMEPS rather than having every runoff model define its own version of this variable that then requires additional changes in CMEPS. The individual runoff models can then remain blissfully unaware of this variable, and any logic related to the variable can be in CMEPS (or in CLM for checking compatibility with CLM_ACCELERATED_SPINUP as noted in point (2)).

ekluzek commented 2 years ago

OK, I'll make a new issue regarding this. I didn't like my solution either, which is why I wanted you to look at it. In the short term we'll bring this in like it is, so that I can do the next CTSM tag. But, we'll decide on a longer term solution and implement that when we are able to.