Skip to content

Conversation

breichl
Copy link

@breichl breichl commented Mar 14, 2025

This is about half of the work needed for issue #816 .

This PR adds a new routine to compute KdWork (the buoyancy flux and its integral) based on N2 and Kd. When provided with N2 at the end of dt_therm in adiabatic_ALE it diagnoses the implied work done by Kd and can split this work into the various processes that contribute to Kd. It splits N2 into salinity and temperature to enhance the information provided by the diagnostic and for diffusivity that is different for heat and salt (e.g. double diffusion). It also allows the total N2 and the salinity and temperature contributions to N2 to be diagnosed and output, which is also useful since all other N2 diagnostics in the model are floored at 0.

It should not be expected to work with KPP, since KPP's nonlocal flux requires additional terms in the buoyancy flux. Therefore, a KPP based diagnostic is not attempted here.

I also did not add a diagnostic for the convection routine since it is not one I exercise, but it could easily be added later.

The diffusivities in set_viscosity can be diagnosed in a similar manner. This PR could be accepted independent of those diagnostics, or it could all be merged at once. I will work on them next.

@breichl breichl requested a review from Hallberg-NOAA March 14, 2025 19:34
@breichl breichl marked this pull request as draft March 14, 2025 19:36
@breichl
Copy link
Author

breichl commented Mar 14, 2025

Looks like it is failing almost all the tests, so I'm marking this as draft and will re-examine ASAP.

@breichl breichl marked this pull request as ready for review March 17, 2025 19:17
@breichl
Copy link
Author

breichl commented Mar 17, 2025

The issues that caused the checks to fail have been addressed.

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These diagnostics have several problems with the choice of rescaled units and introduce an unnecessary dependency on the Boussinesq reference density when in non-Boussinesq mode. Please reconsider the scaling and expressions that are being used here and make sure that all of the diagnostics are passing the full range of dimensional consistency tests.

@breichl
Copy link
Author

breichl commented Apr 11, 2025

@Hallberg-NOAA I think this PR is now ready for re-review, but I'm unclear why it fails the last test (@marshallward any insight?).

The updates are substantial, and the code now fully addresses issue #816 by also including the Kd diagnostics inside set_diffusivity and 2d and volume integrated versions of the diagnostic (it now adds ~80 new total diagnostics to MOM6). It still does not include every Kd that could be used in MOM6, but it should be straightforward for a user to add any new Kd's to this routine in the future. The PR was reorganized by moving most of the diagnostics into the new KdWork module. This reorganization avoids cluttering diabatic_driver.

The code was tested for dimensional consistency in both Boussinesq and non-Boussinesq mode, but I'm open to feedback on choices for scaling.

@marshallward
Copy link
Member

@Hallberg-NOAA I think this PR is now ready for re-review, but I'm unclear why it fails the last test (@marshallward any insight?).

Nothing to do with your PR, the perf output parser is always encountering problems with system libraries. I'll see if it's fixable, but don't let it hold up the PR. (Maybe we should disable this test?)

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for these updates. This is PR looks good to me now, apart from the failing testing for dimensional consistency testing with rescaled Z with the Bflux_dia_diff_dz diagnostic.

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the failing testing for dimensional consistency testing with rescaled Z with the Bflux_dia_diff_dz diagnostic, after which I expect that this will be ready to be merged in.

@breichl
Copy link
Author

breichl commented Apr 14, 2025

Please fix the failing testing for dimensional consistency testing with rescaled Z with the Bflux_dia_diff_dz diagnostic, after which I expect that this will be ready to be merged in.

This issue is fixed now, I had a typo propagate into all of my 3d "dz" diagnostic work arrays. This example is definitely a great case study for the benefit of dimensional consistency tests, this caught a subtle but very important issue!

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my view, this PR is now ready to go.

brandon.reichl and others added 9 commits April 14, 2025 10:28
- Adds the ability to diagnose the implied work and buoyancy flux due to given diffusivities inside of diabatic_ALE
- Adds outputs of N2 after applying diffusion, the implied bouyancy flux, the integrated buoyancy flux, the salinity components of buoyancy flux and N2, the temperature components of buoyancy flux and N2, the ePBL contribution to the buoyancy flux, and double diffusion contribution to the buoyancy flux,
- Only considers the contribution of KPP to the total diffusive flux, the diagnostics should not be used w/ KPP until the non-local term and its contribution are properly accounted for.
- Only considers the contribution of convection to the total diffusive flux, since diagnose its implied work would require accounting for the convective contributions to diffusivity (it would be easily doable following this template in the future, but it is not done here).
- More work is needed to apply these methods to diffusivities that are diagnosed and stored in set_diffusivity.
- This code is not repeated for diabatic_ALE_legacy since the double diffusion component to diffusivity is applied separately, thus complicating the interpretation of these diagnostics in the legacy driver.
- Change buoyancy flux units to W/m3 (W/m2 for vertical integrated terms).
- Correct issues in unit scaling for Boussinesq and non-Boussinesq code.
- Compute N2 from specific volume for non-Boussinesq mode.
brandon.reichl added 11 commits April 14, 2025 10:28
- Kd work diagnostic calculations migrated from MOM_diabatic_driver to MOM_diagnose_KdWork
- Kd work diagnostics stored in new type which contains copies of all requested diffusivities to facilitate storing and moving data around so it is accessible at the conclusion of diabatic_driver.
- All Kd arrays are allocatable that won't be allocated unless needed.
- 2d Vertical and volume integral quantities added.
- Many more Kd work diagnostics added including many from MOM_set_diffusivity, still not exhaustive.
@Hallberg-NOAA Hallberg-NOAA merged commit eb9061d into NOAA-GFDL:dev/gfdl Apr 14, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants