Add Chapman and Terminator configurations for chemistry #151
Conversation
mattldawson
requested review from
jimmielin,
K20shores,
peverwhee,
nusbaume and
boulderdaze
| tuvx => null() | ||
| return | ||
| end if | ||
| if (has_error_occurred( error, errmsg, errcode )) return |
There was a problem hiding this comment.
Is there any reason not to deallocate tuvx when the error occurs? The code below calls the deallocate function upon failure, and this seems inconsistent.
There was a problem hiding this comment.
I removed any deallocations that are already handled by the tuvx_final() function (assuming this gets called by the host model if there is an error). But, I'm not opposed to putting explicit deallocations in the error handling blocks, it just makes for more code. What do you prefer?
There was a problem hiding this comment.
I think the code will be cleaner if we could do that. From the previous discussion with @nusbaume, I learned that in CAM-SIMA, the model run is aborted as soon as a error code other than zero is raised. So at least for now, it will never reaches the final phase. But he also mentioned that if we want different behavior for failures in MUSICA, they could implement alternative fallbacks in CAM-SIMA, such as jumping straight to the final call. Would you prefer that approach? We could bring it up in the next CAM-SIMA meeting.
There was a problem hiding this comment.
I have no strong preference. What if I just called the tuvx_final() function inside these error blocks, just to be sure everything gets deallocated?
There was a problem hiding this comment.
One issue with trying to clean up before the job ends is that often, an error occurs on only one or a subset of atmosphere tasks. Thus, the job is killed without many or most tasks being aware of the situation. Current architectures do not give us a lot of automatic alternatives that do not sacrifice performance. I think cleanup is great in principle and perhaps there is a way to invoke it on all tasks so I think just having it all in the final routine is most likely to be useful in a CAM-SIMA environment.
There was a problem hiding this comment.
I think in general calling the finalize phase before CAM-SIMA aborts is a good idea and should be very easy to do on the CAM-SIMA side, so I went ahead and added an issue for it, which you can find here:
So for this PR if you want to move all of the deallocation steps and any other cleanup that you want to do into tuvx_final then CAM-SIMA should automatically run that before the program stops (at least once the relevant CAM-SIMA PR is made). Hope that helps!
There was a problem hiding this comment.
Thanks guys! I'll leave the deallocations for module-level variables in the final routine then. (Some deallocations will remain in the init function for local variables.)
There was a problem hiding this comment.
Thanks for requesting my review. I had a few questions about the code, but will defer the decision about the location of the data directories to other reviewers.
Regarding these data directories in general, as well as the location of the mechanisms, maybe they could be included as an external, if MICM has a repository of chemical mechanisms and their configurations? I am thinking of this in the context of including future chemical mechanisms and whether it would make sense for them to be included as part of atmospheric_physics or in a separate repository to facilitate future development of chemical mechanisms.
| error ) | ||
| if (has_error_occurred( error, errmsg, errcode )) return | ||
|
|
||
| ! filter out negative photolysis rate constants |
There was a problem hiding this comment.
Is there a reason why TUV-x returns negative rate constants (which would be somewhat concerning?) or is this a generic safety measure? In the latter case I would advocate for this to filter all reaction rates to be non-negative as any negative rates would be catastrophic to the stability of chemistry solvers.
There was a problem hiding this comment.
There is a lot of flexibility in the configuration of TUV-x, so if you provided parameters or datasets that result in, e.g., a negative value in the cross section or quantum yield profiles, it would be possible for TUV-x to return a negative value for photolysis reaction rate constants. I'm not opposed to replacing this filter with an assertion that rate constants remain non-negative, if you think that would be a better choice.
There was a problem hiding this comment.
My preference would be to filter all the rate constants to be non-negative, so all possible cases are covered, but up to you if you'd like to implement that logic in the future!
There was a problem hiding this comment.
Sounds good! I think I would prefer to do this on a case-by-case basis, because as I mentioned in another thread the rate parameters are not all rate constants. Even though the current parameters for rate constant algorithms should all be positive, this may not be true for future algorithms.
Our long-term plan is to version control chemical mechanisms in Chemistry Café, which is still in early stages of development. My only concern at this point about not having the chemistry configurations versioned with the rest of the atmospheric physics code, is that I feel like we may still end up with some dependencies in the source code for physics that requires specific configurations for the chemical system. (Ideally, this won't be the case, but it's unclear, at least to me, at this point.) But, if it makes more sense to create a separate dedicated repo for these until Chemistry Café is ready, I'm open to that. |
There was a problem hiding this comment.
Thanks @mattldawson! All code changes look good to me although I would like to ask for @peverwhee and/or @nusbaume's input on the location of the data directories for chemistry and if it makes sense to make them a separate repository.
|
I don't have a strong opinion on the data given that right now it is pretty small (<10 mb), but if I had to choose then I might vote to make a separate repo that could then be brought into the atmospheric_physics repo as a submodule (which we can version along with atmospheric_physics if need be). This is simply because it will get us used to the idea that the chemistry configurations don't actually live with the physics repo (which sounds like will be the case long-term once the Chemistry Café is ready?), and it will save a few mb in repo space, which can be a big deal for CESM as whole (which I believe is starting to get pretty large). Of course I am happy to be overruled here if that turns out to be too much work, or there is some other issue that I am not thinking of at the moment. Just let me know! |
Sounds good! I will wait to merge until they review or comment. |
This sounds good to me. Should this be a repo in ESCOMP? or NCAR? And would it just be for chemistry configurations or will there be other configuration data for physics also? |
|
I think either organization is fine, although if I had to vote I might say NCAR (as it sounds like the repo will be temporary?). Also I was assuming for now that it would just be chemistry-related configuration data, but if we find that atmospheric_physics or CAM-SIMA is getting too cluttered with physics configuration files then we can certainly reassess that assumption. Hope that helps! |
I moved the chemistry datasets to a separate repo: https://github.com/NCAR/cam-sima-chemistry-data @nusbaume - let me know if this looks ok to you. I'll wait to merge until you have a chance to look at it |
|
@mattldawson The new repo looks great to me, thanks for your help! Feel free to merge this PR in whenever you'd like. |
Tag name (The PR title should also include the tag name): `atmos_phys0_07_000` Originator(s): @jimmielin List all `development` PR URLs included in this PR and a short description of each: * #141 by @mattldawson @boulderdaze * #147 by @peverwhee * #144 by @mwaxmonsky * #151 by @mattldawson * #145 by @jimmielin List all test failures: N/A
Adds configurations for Terminator and Chapman chemistry and basic integration tests.
This includes some data files needed for TUV-x to calculate photolysis rate constants for these mechanisms, but I wasn't sure if this is the best place for them. I'm happy to remove the configuration data from this PR if there is a better place to keep them.
closes #150
Also: