Add water tracers#638
Conversation
Partially addresses ESCOMP#601
This will integrate better with existing infrastructure for processing colon-delimited lists.
Before this change, an empty WATER_TRACERS xml variable was treated as a length-1 list containing an empty string, rather than as a length-0 list.
It looks like these fields were removed from esmFldsExchange_cesm_mod at some point, but weren't removed from fd_cesm.yaml.
We can enable these checks once we add water tracers to ensure that water tracers are being handled consistently with bulk fluxes.
This reverts commit a39d47d. This capability is enabled on this branch so we can back out this temporary commit.
This means we are potentially getting n_tracers multiple times instead of just once, but it allows the code to work in the situation where we don't have any tracer fields and shr_wtracers_mod hasn't been initialized. (I'm not sure if this will arise in practice, but it might.)
This breaks the dependency on this share code for UFS.
This will make it more clear to readers that tracers are handled identically to bulk, and will also make it less likely for the two to get out of sync.
Per suggestion from Mariana Vertenstein
Since this field doesn't have a corresponding bulk field, we need to add some flexibility to the checker method.
Faxa_rain, Faxa_rainc, Faxa_rainl, Faxa_snow, Faxa_snowc, Faxa_snowl
| canonical_units: kg m-2 s-1 | ||
| description: lnd export to river | ||
| # | ||
| - standard_name: Flrl_rofdto |
There was a problem hiding this comment.
This removal of Flrl_rofdto looks weird in the diff - since it looks like I replaced Flrl_rofdto with Flrl_irrig_wtracers - but in fact this is intentional: I removed the no-longer-used Flrl_rofdto and separately added Flrl_irrig_wtracers.
| private :: med_io_def_var_with_atts | ||
| private :: med_io_read_1d_var |
There was a problem hiding this comment.
The changes in this module are not directly related to the addition of water tracers. Rather, they are a follow-on to the additions made to med_io_mod in #634. Those additions were for the sake of adding water tracers, so this is still somewhat related to this overall goal. However, I'd be open to moving these to a separate PR if desired.
| do ns = 1,is_local%wrap%num_icesheets | ||
| call fldbun_reset(toglc_frlnd(ns)%FBlndAccum2glc_g, value=0.0_r8, rc=rc) | ||
| if (chkerr(rc,__LINE__,u_FILE_u)) return | ||
| end do |
There was a problem hiding this comment.
This call to fldbun_reset was duplicated below, so I removed this unnecessary call.
|
I have run more extensive testing (noted in my edited top-level comment) and given the diffs a final, quick once-over myself. So this is now (finally!) ready for review. |
The handling of water tracers / isotopes is being overhauled and will look different moving forward. See also ESCOMP/CMEPS#638
The handling of water tracers / isotopes is being overhauled and will look different moving forward. See also ESCOMP/CMEPS#638
nusbaume
left a comment
nusbaume
left a comment
There was a problem hiding this comment.
Thanks for all of this great new tracer infrastructure @billsacks! The only real concern I have is making sure the water tracer test contains at least one initial ratio that is less than one, as that will be closer to production run configurations. All the other comments are either just questions or totally optional code changes.
| is_tracer = wtracers_is_wtracer_field(field_names(i)) | ||
| if (is_tracer) then | ||
| ! Field is a water tracer; assume a single ungridded dimension | ||
| n_tracers = wtracers_get_num_tracers() |
There was a problem hiding this comment.
Would it be better to pull this function outside of the n_fields loop? I realize this might mean adding a shr_wtracers_present check first to avoid running into an error.
I am mostly requesting this because it might look like there is a different number of tracers for different fields, which I don't believe can be the case (?).
There was a problem hiding this comment.
Here was my rationale for moving this fetch of n_tracers into the is_tracers conditional (which means putting it inside the loop):
This means we are potentially getting n_tracers multiple times instead of just once, but it allows the code to work in the situation where we don't have any tracer fields and shr_wtracers_mod hasn't been initialized. (I'm not sure if this will arise in practice, but it might.)
I considered pulling this out of the loop and instead putting it in a conditional on whether shr_wtracers_mod has been initialized - otherwise setting n_tracers to 0. But the problem with that is: If this code is called from a path where there are water tracers, but shr_wtracers_mod hasn't yet been initialized, the resulting error will be more cryptic.
So I lean towards keeping this code as it is. But if you think it would be helpful, I can add a comment - probably something similar to my above paragraph starting with "This means".
Let me know your thoughts.
There was a problem hiding this comment.
Thanks for the explanation @billsacks! I would much rather be safe than slightly more readable, so I'm happy to leave this line where it is. Maybe I would vote to add the comment in, though, assuming it doesn't clutter the code or anything? That way four years from now when I look at this code again I will remember this conversation.
Add more infrastructure for water tracers, needed by CMEPS. The biggest addition here is a routine to check tracer ratios against expectations, with associated unit tests. There are also a few other small additions to shr_wtracers_mod as well as some other additions needed for the unit test build. Testing done: In the context of cesm3_0_alpha08o, with this branch along with ESCOMP/CMEPS#638, ran CESM prealpha tests on derecho and izumi, and aux_glc tests on derecho, all with baseline comparisons. All failures also failed in the baseline except for `SMS_D_Ln9_P1536x1.ne0CONUSne30x8_ne0CONUSne30x8_mt12.FHIST.derecho_intel.cam-outfrq9s` (which seemed to fail due to ESCOMP/CTSM#3772, which is a sporadic issue), and all baseline comparisons passed. (Note: full testing was done before final commits addressing review points; only minimal additional testing was done after those final commits.)
|
@nusbaume - thank you very much for your thoughtful review! I have addressed most of your suggestions. One outstanding point for discussion is #638 (comment) . Also possibly opening an issue for #638 (comment) ? Beyond that, I'll just work to get the workflow passing... e.g., resolving warnings. |
I thought this direct dependence on shr_wtracers_mod would be okay here, but this breaks the GitHub Action build. I think what's happening is that the CMake-based build builds esmFldsExchange_cesm_mod but doesn't include the CESM share code (instead getting the share code from CDEPS, I think?).
|
@nusbaume - I have made some other minor changes to get the standalone build to work in the GitHub action. So this is ready for re-review whenever you get a chance... I think the only outstanding things are what I mentioned in my earlier comment. |
nusbaume
left a comment
nusbaume
left a comment
There was a problem hiding this comment.
Thanks for the code changes and responses @billsacks! Everything looks great to me now.
3 participants
Description of changes
Core changes needed to add water tracers in CESM, for the sake of water isotopes and other purposes.
Major pieces of this include:
Some pieces are not yet implemented:
Depends on ESCOMP/CESM_share#82
Specific notes
Contributors other than yourself, if any:
CMEPS Issues Fixed (include github issue #):
Are changes expected to change answers? (specify if bfb, different at roundoff, more substantial) Changes answers for X compsets (due to some changed counter logic and maybe other things); should be bit-for-bit for all other compsets
Any User Interface Changes (namelist or namelist defaults changes)? Adds xml and config variables controlling water tracers
Testing performed
In the context of cesm3_0_alpha08o, with this branch along with ESCOMP/CESM_share#82, ran CESM prealpha tests on derecho and izumi, and aux_glc tests on derecho, all with baseline comparisons. All failures also failed in the baseline except for
SMS_D_Ln9_P1536x1.ne0CONUSne30x8_ne0CONUSne30x8_mt12.FHIST.derecho_intel.cam-outfrq9s(which seemed to fail due to ESCOMP/CTSM#3772, which is a sporadic issue), and all baseline comparisons passed. (Note: full testing was done before final commits addressing review points; only minimal additional testing was done after those final commits.)