Skip to content

Fix Oyr.osaltdiff discovery with yearly-only aggregation#441

Draft
rbeucher wants to merge 1 commit into
mainfrom
fix-199-osaltdiff-oyr-resampling
Draft

Fix Oyr.osaltdiff discovery with yearly-only aggregation#441
rbeucher wants to merge 1 commit into
mainfrom
fix-199-osaltdiff-oyr-resampling

Conversation

@rbeucher

@rbeucher rbeucher commented Jun 18, 2026

Copy link
Copy Markdown
Member

Summary

This PR addresses #199 by allowing Oyr.osaltdiff to be generated from available monthly ACCESS output, while ensuring aggregation behavior only applies to yearly ocean requests.

What changed

  • Added frequency-specific per-variable file pattern support in file discovery.
  • Added an Oyr-specific override for osaltdiff in ACCESS-ESM1-6 mappings so Oyr can discover monthly salt_vdiffuse_impl files.
  • Wired ocean driver/CMORiser resampling controls so ocean yearly tables auto-enable resampling.
  • Ensured monthly ocean tables do not auto-enable resampling.
  • Added unit tests for:
    • frequency-specific file pattern override behavior and fallback,
    • yearly ocean auto-resampling enablement,
    • monthly ocean non-auto-enable behavior.

Why this resolves #199

Issue #199 reports missing salt_vdiffuse_diff_cbt for Oyr.osaltdiff. In current outputs, salt_vdiffuse_impl is available monthly. This PR enables Oyr.osaltdiff to discover that monthly input and aggregate to yearly only when yearly output is requested.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.4%. Comparing base (bf7b1d6) to head (04f183a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #441   +/-   ##
=====================================
  Coverage   76.4%   76.4%           
=====================================
  Files         31      31           
  Lines       5909    5913    +4     
  Branches    1092    1094    +2     
=====================================
+ Hits        4515    4519    +4     
  Misses      1154    1154           
  Partials     240     240           
Flag Coverage Δ
unit 76.4% <100.0%> (+<0.1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

@rbeucher

Copy link
Copy Markdown
Member Author

Scientific caveat — do not merge yet

After opening this PR, further investigation raised a concern about the correctness of the osaltdiff mapping.

Current mapping: salt_vdiffuse_impl (implicit vertical diffusion of Practical Salinity)
CMIP target: tendency_of_sea_water_salinity_expressed_as_salt_content_due_to_parameterized_dianeutral_mixing
Ideal source (per issue #199): salt_vdiffuse_diff_cbt — not present in current outputs

Why these may differ

osaltdiff is defined as a strictly dianeutral (across-density-surface) mixing tendency. salt_vdiffuse_impl is the implicit vertical (z-coordinate) diffusion term from the MOM5 solver. These are related but not equivalent:

  • Dianeutral mixing is referenced to neutral/isopycnal surfaces; implicit vertical diffusion is referenced to model z-levels.
  • The implicit diffusion term may include contributions from parameterisations that are not purely dianeutral.
  • salt_vdiffuse_diff_cbt would isolate the diffusivity-driven (CBT) component that is the intended physical analogue.

Status

This PR provides the infrastructure (frequency-specific file discovery, yearly-only aggregation wiring) which is correct and reusable regardless of which source variable is ultimately used. However, the mapping itself (salt_vdiffuse_impl → osaltdiff) should be validated by an ocean scientist or replaced with salt_vdiffuse_diff_cbt once that diagnostic is added to ACCESS-ESM1.6 output.

Keeping this open for review/discussion but not ready to merge until the source variable choice is confirmed.

@rbeucher rbeucher marked this pull request as draft June 18, 2026 05:32
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.

Missing salt_vdiffuse_diff_cbt for Oyr.osaltdiff

1 participant