Add KPP nonlocal term to passive tracers #202
Conversation
When running with CS%useKPP = .true., the new function call_tracer_KPP_NonLocalTransport() controls whether each tracer module applies the KPP nonlocal term to its tracers. This has been implemented in the CFC tracer module as a proof of concept, but has not been extended to other modules yet.
|
A couple open questions:
Once the general approach is approved, I am happy to extend this to other tracer modules. Also, it looks like I don't have permission to add reviewers to this PR... but I would suggest including some of the GFDL developers to help answer the above questions. |
This commit should pass the doxygen CI test
Codecov Report
@@ Coverage Diff @@
## dev/ncar #202 +/- ##
=========================================
Coverage 28.87% 28.87%
=========================================
Files 242 243 +1
Lines 71863 71875 +12
=========================================
+ Hits 20749 20756 +7
- Misses 51114 51119 +5
Continue to review full report at Codecov.
|
| enddo | ||
|
|
||
| ! Update tracer due to non-local redistribution of surface flux | ||
| if (applyNonLocalTrans) then |
There was a problem hiding this comment.
Is it desirable to call KPP_NonLocalTransport_passive_tracers when applyNonLocalTrans = False?
If yes, perhaps dtracer should only be calculated if applyNonLocalTrans = True or its diagnostic has been requested. If not, it would make more sense if this condition was outside of KPP_NonLocalTransport_passive_tracers.
The same comment applies for KPP_NonLocalTransport_temp and KPP_NonLocalTransport_saln.
There was a problem hiding this comment.
The real issue is that I haven't plumbed in any diagnostics yet - for KPP_NonLocalTransport_temp and KPP_NonLocalTransport_saln, if applyNonLocalTrans is false then you still compute the terms and post the data, you just don't add the terms to the tracer quantities themselves... but when I first started working on this branch I got a little stuck trying to figure out how to include diagnostics and then I didn't come back to that before opening this PR
KPP_NonLocalTransport_passive_tracers() now posts surface flux and the tendency due to the KPP nonlocal term if those diagnostics are requested in output. There is probably a better way to keep track of the diagnostic indices, but for now I rely on the fact that the tracer registry is in each tracer module's control structure and we just need to track the indices of CS%Tr_reg%Tr(:) corresponding to the desired tracers
|
After talking to @klindsay28 and @ashao, I think the next step is to reduce repeated code by having We also discussed the best way to pass the diagnostic IDs in to the routine, and one possibility is to just pass the entire |
Introduce CFC_tracer_metadata type to remove per-tracer values in the control structure (such as CS%CFC11_name and CS%CFC12_name)
KPP_NonLocalTransport_temp() and KPP_NonLocalTransport_saln now call KPP_NonLocalTransport() (which is also called from the MOM_CFC_cap); this reduces duplicate code and also adds logic to only compute the nonlocal terms if they are either (a) being applied, or (b) being posted to diagnostics.
|
@gustavo-marques you had asked about re-running some of the CFC experiments with this branch, and I think functionally everything is ready for you to do that. Unless I have a bug somewhere subsequent pushes to this branch should be bit-for-bit refactoring or commits that add support for these nonlocal terms to other tracer modules. When you run, you need to set in your override; the default is |
KPP_NonLocalTransport() now handles posting the budget term (if requested), and the id for that term is available in Tr_reg%Tr(:) so it can be passed from MOM_CFC_cap as well
The three per-tracer diagnostics added to the tracer registry (surface flux, non-local term, and budget component of NLT) are only defined if KPP is enabled in the diabatic driver. Note that this requires a change to MOM.F90, which may not be desired -- the alternative is to always register the KPP diagnostics whether KPP is enabled or not.
Added tr_ptr to the thermo_var_ptrs type, and updated MOM.F90 to have them point to T & S correctly. There is some kludgy logic in register_tracer_diagnostics() to register the T & S diagnostics correctly, but I'll clean that up in a future commit. In the mean time, I was able to move some of the diagnostic IDs out of the KPP control structure.
Had circular dependencies somewhere, so I needed to move tracer_type out of MOM_tracer_registry.F90. I creeated MOM_tracer_types.F90 for both tracer_type and tracer_registry_type. Note that MOM_tracer_registry uses both of these types AND makes them public, so other modules can continue to use them from MOM_tracer_registry instead of needing to be changed to use them from the new module. Also added doxygen comments to tr_T and tr_S in thermo_var_ptrs to pass the documentation CI test.
This implementation mimics the CFC implementation, and with it I can verify that the nonlocal term for passive tracers is NOT set up in the same manner as it is for T & S -- there are very large differences between pseudo-salt and real salt when PSEUDOSALT_APPLY_NONLOCAL_TRANSPORT = True
Refactored both the MOM_CFC_cap and pseudo_salt_tracer modules so that the
KPP_NonLocalTransport() call is done in {}_column_physics prior to the
tracer_vertdiff() call. This is an improvement over the previous code, but
there are still significant differences between pseudo_salt and so
Added KPP_salt_flux to forcing type, and have it point to CS%KPP_salt_flux in the various diabatic drivers. After one day run, seeing no difference between so and pseudo-salt when KPP is enabled.
|
I can't reproduce the failures from edit: found the "re-run all jobs" button on my branch, though it isn't available to me through the PR. Currently re-running the test |
|
Okay, I reran the failing test on my branch and it passed on the second attempt. phew! With that test passing, I think this PR is ready for more review. What I've done so far to test the changes:
I've also added the nonlocal terms to |
I was posting a diagnostic that relied on a local variable before computing the local variable, so the diagnostic was 0 everywhere. I also removed a couple of comments that no longer apply
|
@mnlevy1981 , can you point me to a case with negative CFC concentrations? |
|
|
The following fields in that case's available_diags.000000 file have empty strings for units: |
Two lines were failing doxygen test because they were too long
|
@gustavo-marques I've committed all the changes we discussed during last Friday's code review. When you get a chance (I know OMWG is happening this week, so no rush!), could you run the bigger stand-alone test suite? I still need to edit the first post in this PR as well, but I think the code modifications are done. Also, at one point you mentioned re-running the CFC experiments - would you mind spinning up one of those runs as well so we can get a feel for the effect the nonlocal terms have on the CFCs? |
|
@mnlevy1981, I have run the MOM6-examples test suit and this PR does not change answers w.r.t. the latest main (9cb9304). I also re-started the two CFC simulations (z* and hybrid, using restarts from year 1939). I will touch base with you early next week to make sure all is good in these runs. Thanks! |
Here's some additional info on the circular dependency being avoided. If the types being used by |
|
As of 900f6cf, I've merged in Tr%id_NLT_budget = register_diag_field('ocean_model', Tr%NLT_budget_name, &
diag%axesTL, Time, &
trim(flux_longname)//' content change due to non-local transport, as calculated by [CVMix] KPP', &
conv_units, conversion=Tr%conv_scale, v_extensive=.true.)But for salinity, if (Tr%conv_scale == 0.001*GV%H_to_kg_m2) then
conversion = GV%H_to_kg_m2
else
conversion = Tr%conv_scale
end if
Tr%id_NLT_budget = register_diag_field('ocean_model', Tr%NLT_budget_name, &
diag%axesTL, Time, &
trim(flux_longname)//' content change due to non-local transport, as calculated by [CVMix] KPP', &
conv_units, conversion=conversion*US%s_to_T, v_extensive=.true.)
endifI think we actually want one of the following two potential solutions:
I know others are waiting on this PR to continue there work, though; while I don't typically advocate for merging terrible code perhaps the best path forward is to take this as-is and immediately open an issue ticket / start working on a fix For testing, I generated a baseline for I also ran a one-off test where I compared output from a run with CFCs and pseudosalt to a baseline generated by my branch before merging The Full disclosure, there were two surprises in the testing output. First, there were a couple of The The other surprise is that This seems like an issue with how the test suite is configured, but it looks like these tests will continue to fail even when all the history output found is bit-for-bit. |
|
After talking with @klindsay28, I have one more commit coming that will clean up a couple of comments and then this will be ready to be reviewed / merged. It might make sense to squash-merge it instead of keeping the 32 commit history of the branch. Re: the convergence scale kludge I mentioned in my previous commit -- we want to leave it in place for this PR but @klindsay28 will remove it when he cleans up the diagnostic framework. |
Units were incorrect in comment describing cfc11_flux and cfc12_flux. Also, updated comment surrounding the kludge in the conversion term when registering the NLT_budget diagnostics for each tracer.
3e4f827 was the "one more commit". All CI tests are passing, |
| @@ -251,7 +251,7 @@ module MOM_diabatic_driver | |||
| real, allocatable, dimension(:,:,:) :: KPP_buoy_flux !< KPP forcing buoyancy flux [L2 T-3 ~> m2 s-3] | |||
| real, allocatable, dimension(:,:) :: KPP_temp_flux !< KPP effective temperature flux | |||
| !! [degC H T-1 ~> degC m s-1 or degC kg m-2 s-1] | |||
| real, allocatable, dimension(:,:) :: KPP_salt_flux !< KPP effective salt flux | |||
| real, pointer, dimension(:,:) :: KPP_salt_flux => NULL() !< KPP effective salt flux | |||
There was a problem hiding this comment.
Is there a reason to convert this array to a pointer? GFDL has recently replaced many pointer declarations in MOM6 as either allocatables or locals for improving the performance: NOAA-GFDL#5
So, unless there is a limitation for using an allocable array herer, I suggest keeping it an allocatable array
There was a problem hiding this comment.
The salt flux is also needed in the pseudosalt module, and so I added a pointer for it to forcing_type and then the diabatic driver ensures the forcing_type pointer points to this field.
There was a problem hiding this comment.
Thanks. Now that makes me wondering why KPP_salt_flux is a part of diabatic_CS. If it was a member of KPP_CSp instead, then you wouldn't need to make it a pointer or add it to forcing_type. The same goes for KPP_NLTheat and other KPP_* data above. Why are they not a part of the KPP module? Anyway, these can be deemed outside the scope of this PR, but KPP_salt_flux being a part of KPP_CSp would simplify this PR a bit.
There was a problem hiding this comment.
Thanks. Now that makes me wondering why
KPP_salt_fluxis a part ofdiabatic_CS. If it was a member ofKPP_CSpinstead, then you wouldn't need to make it a pointer or add it toforcing_type. The same goes forKPP_NLTheatand otherKPP_*data above. Why are they not a part of the KPP module? Anyway, these can be deemed outside the scope of this PR, butKPP_salt_fluxbeing a part ofKPP_CSpwould simplify this PR a bit.
I think this would be an interesting discussion for one of the MOM meetings with GFDL, or maybe via an issue ticket on the mom-ocean repo? I agree that refactoring this in a way that would let us pass the data to pseudosalt without the addition to forcing_type would be an improvement over what is in this PR
|
The design of tracers in MOM6 is that tracer modules are responsible for
the memory for tracer values, they pass this memory to `register_tracer`,
and `register_tracer` points the `t` component of the `type(tracer_type)`
object to this memory. I'm not seeing how to preserve this paradigm while
changing the `t` component to an allocatable array. That said, it's
possible that some of the other components of the `tracer_type` type could
be changed to be allocatable arrays, though I agree with Mike that doing
that is beyond the scope of this PR.
…On Wed, Mar 2, 2022 at 3:54 PM Michael Levy ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/tracer/MOM_tracer_types.F90
<#202 (comment)>:
> @@ -0,0 +1,131 @@
+!> Pulls out types previously defined in MOM_tracer_registry
+!! To avoid circular dependencies
+module MOM_tracer_types
+
+implicit none ; private
+
+#include <MOM_memory.h>
+
+!> The tracer type
+type, public :: tracer_type
+
+ real, dimension(:,:,:), pointer :: t => NULL() !< tracer concentration array [conc]
This is showing up as new code because I moved it out of
MOM_tracer_registry.F90, but t is currently a pointer on dev/ncar so I
think changing it is beyond the scope of this PR:
https://github.com/NCAR/MOM6/blob/dev/ncar/src/tracer/MOM_tracer_registry.F90#L40
—
Reply to this email directly, view it on GitHub
<#202 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADWZPO26L3BCC2BYZQY55GTU57WT7ANCNFSM5JCPM7WQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Code review pointed out that netSalt had been removed from the forcing type on dev/ncar, but I inadvertantly added it back in. Also removed commented out lines referring to the now-removed variable.
|
728fe16 addressed @klindsay28's comment about |
There was a problem hiding this comment.
I agree that the remaining points that I raised are beyond the scope of this PR. So I think it is ready to be merged.
I like the idea of merging this PR as a squash merge. This would reduce the clutter from the many commits, and I only see a minimal benefit from separating commits. The only commit that I see worth considering separately is the movement of tracer_type from MOM_tracer_registry.F90 to MOM_tracer_types.F90, and I don't see its separation as necessary. I think the main challenge is to write a commit message for the squash merge that fully describes what the PR does.
Two items to consider beyond this PR:
- @alperaltuntas's suggestion of moveng KPP specific fields, like
KPP_salt_flux, from theforcingtype to theKPP_CStype. - Add a rotation test, with KPP enabled, to aux_mom and/or github actions testing. There are rotation tests in github actions testing, but I don't think any of those enable KPP.
I also think that this should be a squash-merge; a reasonable commit message for it would be I'm open to suggestions / improvements on that, but I think it touches all the key points of this PR. |
|
@mnlevy1981, maybe mention in the overall message that there is cleanup in the MOM_CFC_cap module that replaces some (nearly) repeated lines of code with loops over the module's tracers. |
Good catch! I have edited my comment containing the commit message to include these changes, because that seems less confusing than having multiple drafts of the commit message in different comments in this PR. It also reminds me of some clean-up that should wait for a future PR, but it doesn't really make sense to have tracer concentration in a type named |
I was thinking about addressing this while waiting for the updates needed for @gustavo-marques to run the stand-alone regression tests, and one potential solution would be to just rename diff --git a/src/tracer/MOM_CFC_cap.F90 b/src/tracer/MOM_CFC_cap.F90
index a80221e4a..22788c896 100644
--- a/src/tracer/MOM_CFC_cap.F90
+++ b/src/tracer/MOM_CFC_cap.F90
@@ -36,9 +36,8 @@ module MOM_CFC_cap
integer, parameter :: NTR = 2 !< the number of tracers in this module.
-!> Contains per-tracer metadata and a pointer to Tr in Tr_reg
-type, private :: CFC_tracer_metadata
- ! The following vardesc types contain a package of metadata about each tracer.
+!> Contains the concentration array, a pointer to Tr in Tr_reg, and some metadata for a single CFC tracer
+type, private :: CFC_tracer_data
type(vardesc) :: desc !< A set of metadata for the tracer
real :: IC_val = 0.0 !< The initial value assigned to the tracer [mol kg-1].
real :: land_val = -1.0 !< The value of the tracer used where land is masked out [mol kg-1].
@@ -46,7 +45,7 @@ module MOM_CFC_cap
integer :: id_cmor !< Diagnostic ID
real, pointer, dimension(:,:,:) :: conc !< The tracer concentration [mol kg-1].
type(tracer_type), pointer :: tr_ptr !< pointer to tracer inside Tr_reg
- end type CFC_tracer_metadata
+ end type CFC_tracer_data
!> The control structure for the CFC_cap tracer package
type, public :: CFC_cap_CS ; private
@@ -61,7 +60,7 @@ module MOM_CFC_cap
!! the timing of diagnostic output.
type(MOM_restart_CS), pointer :: restart_CSp => NULL() !< Model restart control structure
- type(CFC_tracer_metadata), dimension(2) :: CFC_metadata !< per-tracer parameters / metadata
+ type(CFC_tracer_data), dimension(2) :: CFC_data !< per-tracer parameters / metadata
end type CFC_cap_CS
contains(I didn't show every single Does anyone have strong feelings about whether or not I should push this change? My plan would be to build and run a single case with CFCs enabled to ensure bit-for-bit against a baseline rather than the full |
CFC_tracer_metadata contains data that isn't actually metadata (the actual concentration array; a pointer to the tracer_type in Tr_reg); this commit cleans up the naming of both the type and the variable of the type used in the control structure (it is now CFC_data instead of CFC_metadata)
The main doxygen description of MOM_tracer_registry.F90 claimed that tracer_registry_type was defined in that module, but it was moved to MOM_tracer_types.F90
|
This PR does not change answers for MOM6-examples when compared against MOM6/main. |
Barotropic weight vectorization
When running with
CS%useKPP = .true., the new functioncall_tracer_KPP_NonLocalTransport()controls whether each tracer module appliesthe KPP nonlocal term to its tracers. This has been implemented in the CFC
tracer module as a proof of concept, but has not been extended to other modules
yet.
For CFCs, there is a new parameter
CFC_APPLY_NONLOCAL_TRANSPORTthat, when true, appliesthe nonlocal term to the CFC tracers (akin to
APPLY_NONLOCAL_TRANSPORTand T & S)