Skip to content

Conversation

breichl
Copy link

@breichl breichl commented Mar 18, 2025

This addresses issue #858.

  • Previously the global/area mean temperature and salinity outputs were the model prognostic temp/salt, but were always reported as potential temperature and practical salinity.
  • This gives the correction for those outputs so it is actually potential temperature and practical salinity, even when the model is conservative and absolute.
  • The option is also added to output the averaged conservative temperature and absolute salinity variables when those are the prognostic variables.
  • The option to output conservative temp and absolute salinity when model is potential temp and practical salinity is not added.
  • This PR will change the horizontally averaged T&S output data for cases that used conservative temp and absolute salinity by applying the correction before the averaging.

@breichl
Copy link
Author

breichl commented Mar 18, 2025

I believe the checks that failed are expected. My inclination is to not wrap "bugfix" changes around these unless necessary since it is only diagnostics that change. But I'm unsure if it will impact other users, I'm not sure if anyone else has started using the TEOS-10 EOS.

@marshallward
Copy link
Member

It looks to me like they are identical but in a different order?

@breichl
Copy link
Author

breichl commented Mar 18, 2025

It looks to me like they are identical but in a different order?

Oh good point, yes in these runs they should be identical. They will only change for cases that used the conservative temp/absolute salinity.

brandon.reichl added 2 commits March 19, 2025 06:47
- Previously the global/area mean temperature and salinity outputs were the model prognostic temp/salt, but were always reported as potential temperature and practical salinity.
- This gives the correction for those outputs so it is actually potential temperature and practical salinity, even when the model is conservative and absolute.
- The option is also added to output the averaged conservative temperature and absolute salinity variables when those are the prognostic variables.
- The option to output conservative temp and absolute salinity when model is potential temp and practical salinity is not added.
@breichl breichl force-pushed the ConT_AbsS_meandiagnostics branch from 5e4beb6 to a11329d Compare March 19, 2025 10:47
Copy link

codecov bot commented Mar 19, 2025

Codecov Report

Attention: Patch coverage is 40.00000% with 36 lines in your changes missing coverage. Please review.

Project coverage is 38.06%. Comparing base (c73ad58) to head (a11329d).

Files with missing lines Patch % Lines
src/diagnostics/MOM_diagnostics.F90 40.00% 36 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           dev/gfdl     #860      +/-   ##
============================================
- Coverage     38.07%   38.06%   -0.01%     
============================================
  Files           298      298              
  Lines         87946    87988      +42     
  Branches      16508    16520      +12     
============================================
+ Hits          33482    33490       +8     
- Misses        48411    48446      +35     
+ Partials       6053     6052       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

We decided to let this in for now and then (hopefully) try to update the testing to handle this before it gets shipped to mom-ocean.

@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/jobs/151913 ✔️ 🟡

New diagnostics added to available_diags.*.

@marshallward marshallward merged commit 792a061 into NOAA-GFDL:dev/gfdl Mar 20, 2025
8 of 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.

2 participants