Change the user interface so that closure tracers are automatically added #4788
Conversation
|
I think this is also a better design because it prevents potential conflict between BGC tracers and closure tracers. |
Co-authored-by: Navid C. Constantinou <[email protected]>
It's breaking if we keep the error, but we can use a warning instead (perhaps for some limited period of time) |
|
Definitely agree with this change! Agreed also with not bumping the version. |
| "tracer names $(closure_tracer_names) associated with $(summary(closure)).", '\n', | ||
| "The names $(closure_tracer_names) cannot be specified explicitly", '\n', | ||
| "or be the names of biogeochemical tracers.") | ||
| throw(ArgumentError(msg)) |
There was a problem hiding this comment.
I think this should be a warning, because the user should be able to manually add tracers even if using a closure that needs that tracer. This is in the spirit of making the code understandable and making sure users know what they are doing. We had this conversation some time ago in CliMA/ClimaOcean.jl#125, where we decided that it is propedeutic to require the users to manually add tracers that are needed and not automatically add them. I think that conversation still holds here. I like the idea of automatically adding tracers (like I was stating in that conversation), but I wouldn't preclude the possibility of explicitly adding tracers that are needed for parameterizations.
There was a problem hiding this comment.
The reason we need to throw an error is because users may want to add passive tracers whose names conflict with the active closure tracers. I think this is a significant enough issue to warrant an error.
I don't think there is a benefit to allowing users to write tracers = :e and have the same effect as not adding tracers at all. Rather, I think this behavior is confusing and surprising.
Regarding the conversation in ClimaOcean:
We had this conversation some time ago in CliMA/ClimaOcean.jl#125, where we decided that it is propedeutic to require the users to manually add tracers that are needed and not automatically add them.
This PR overturns that decision: after this PR, users will not manually add closure tracers.
This PR changes how model constructors work so that "auxiliary tracers" associated with closures --- which right now are just turbulent kinetic energy (TKE) for
CATKEVerticalDiffusivity, and TKE + TKE dissipation (denoted \epsilon) forTKEDissipationVerticalDiffusivityare automatically added to models.The rationale is that this is the interface we want in order to support "one line" changes between closures. Otherwise, users have to introduce their own logic (boilerplate) into scripts to switch between closures.
The original rationale for requiring explicit tracers is that this "self-documents" the tracer names. This is helpful for output. Another rationale is if users need to write forcing functions or set initial conditions for closure auxiliary variables. The latter two are rarely needed. The former remains, but can be solved with ordinary documentation. I think the pros of automatically adding closure auxiliary variables outweighs the cons.