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

Pass tdepth as needed for hillslope model #46

Merged
merged 20 commits into from
Nov 2, 2021

Conversation

ekluzek
Copy link
Contributor

@ekluzek ekluzek commented Oct 28, 2021

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 ekluzek self-assigned this Oct 28, 2021
@ekluzek ekluzek requested a review from billsacks November 2, 2021 18:58
@ekluzek
Copy link
Contributor Author

ekluzek commented Nov 2, 2021

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

config['clm_accel'] = case.get_value("CLM_ACCELERATED_SPINUP")
if ( config['clm_accel'] != "off" ):
config['mosart_mode'] = "NULL"
case.set_value( "MOSART_MODE", config['mosart_mode'] )
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 probably should do this in buildlib rather than here.

@billsacks
Copy link
Member

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
Copy link
Contributor Author

ekluzek commented Nov 2, 2021

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.

@ekluzek ekluzek merged commit fb170a8 into ESCOMP:master Nov 2, 2021
@ekluzek ekluzek deleted the pass_tdepth branch November 2, 2021 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants