-
Notifications
You must be signed in to change notification settings - Fork 94
new atmocn flux modules to replace shr_flux_mod and removal of water isotope references #602
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
base: main
Are you sure you want to change the base?
new atmocn flux modules to replace shr_flux_mod and removal of water isotope references #602
Conversation
… refactored in upcoming work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a significant cleanup of dead isotope code and reorganization of atmocn flux code. Thank you!
@jedwards4b @billsacks should the CDEPS code be cleaned up too? |
@mvdebolskiy I will defer to @billsacks since he will be adding the new isotope work. |
It appears that there are significant changes required in shr_flux_mod.F90, for example all of the loc_* variables need to be made public. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this great refactor and cleanup, @mvertens - both of the shr_flux code and of the unused/untested water isotope code (which I'll be replacing soon with a new, more flexible version)!
I have just two inline comments. There's a simple one about adding an else
clause that would be good to have. The other one - adding named constants - is something I see as optional, so if you don't want to take the time to do that, that's okay.
!! ocn_surface_flux_scheme = 0 : Large and Pond | ||
!! = 1 : COARE algorithm | ||
!! = 2 : UA algorithm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I know that referring to these by their numbers is a pre-existing thing that you've maintained, so this is more a nice-to-have thought rather than something I see as a requirement for this PR.)
It would be great if there could be named parameters for these values, like:
integer, parameter :: ocn_flux_scheme_undefined = -1
integer, parameter :: ocn_flux_scheme_large_and_pond = 0
integer, parameter :: ocn_flux_scheme_coare = 1
integer, parameter :: ocn_flux_scheme_ua = 2
So that, e.g., all the places that check:
if (ocn_surface_flux_scheme == 1) then
could be replaced with:
if (ocn_surface_flux_scheme == ocn_flux_scheme_coare) then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@billsacks - thanks for this suggestion. I have implemented this - but without the ocn_flux_scheme_undefined option - since this is coming in as an input argument.
end if | ||
end do | ||
|
||
end if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an else
clause that aborts with an error message here for an unexpected ocn_surface_flux_scheme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@billsacks - I have implemented this.
I'm adding @fischer-ncar as a reviewer so we know to wait to merge this until some testing has passed successfully on the CESM side. |
Regarding these points:
I'm always kind of uncomfortable with refactoring that isn't covered by tests. It sounds like the diurnal scheme may be hard to test; if so, we can let that one go. What about the UA scheme? How feasible is it to run at least one test of that scheme in either NorESM or CESM to ensure that's still bit-for-bit? |
I will be cleaning up the CDEPS code in the coming weeks/months as I put in place a flexible water tracer/isotope implementation. So unless there's a reason to do that now, I think it's fine to wait on that until I rework it. |
I'll do a test with UA in NorESM to verify bfb. That should not be difficult to do. I'll post the result here and in the NorESM PR. |
I was not comfortable with this - but CAM uses this routine - and I did not want to have a CAM PR to accompany this PR - so for the sake of backwards compatibility I made them public. Which contradicts the loc_ prefix. |
Just so it's recorded here, too: see NorESMhub#43 (comment) for verification of the UA scheme in NorESM. |
Description of changes
new atmocn flux modules to replace shr_flux_mod and removal of water isotope references
Specific notes
Removed all references to the water isotope code. This code was never tested and furthermore the implementation of water isotopes in CESM3 is being handled in a new project at NCAR and this code is being totally rewritten by @billsacks. In conversations with @billsacks - he was comfortable with my removal of this code.
The different subroutine in
shr_flux_mod.F90
have now been split into 4 separate routines plus a top level driver:The configuration constants are still set in
shr_flux_mod.F90
for backwards compatibility, since CAM still calls this routine.Several notes:
Contributors other than yourself, if any: None
CMEPS Issues Fixed: None
Are changes expected to change answers? bfb
Any User Interface Changes: None
Testing performed
Only NorESM testing was done and this is summarized here. However, this should translate hopefully to bfb capability in CESM - but separate testing must be done in the CESM code base,
Using noresm3_0_beta03a - ran