Skip to content

Add ocean model conservation checks#437

Merged
cbegeman merged 17 commits intoE3SM-Project:mainfrom
cbegeman:add-conservation-checks
Dec 19, 2025
Merged

Add ocean model conservation checks#437
cbegeman merged 17 commits intoE3SM-Project:mainfrom
cbegeman:add-conservation-checks

Conversation

@cbegeman
Copy link
Copy Markdown
Collaborator

@cbegeman cbegeman commented Dec 17, 2025

This PR adds only the conservation checks done in MPAS-Ocean, with commented-out placeholder code for the non-boussinesq mass conservation check for Omega.

Checklist
* [ ] User's Guide has been updated

  • Developer's Guide has been updated
  • API documentation in the Developer's Guide (api.md) has any new or modified class, method and/or functions listed
  • Documentation has been built locally and changes look as expected
  • Testing comment in the PR documents testing used to verify the changes
    * [ ] New tests have been added to a test suite

@cbegeman
Copy link
Copy Markdown
Collaborator Author

@xylar Can you provide really broad feedback on the structure of this PR, particularly

  • Treating conservation checks as an instance of a property check
  • verify_properties as argument to add_output_file
  • verify_properties as a method of step which is overridden by OceanModelStep
  • whether to rename verify to check everywhere

@xylar
Copy link
Copy Markdown
Collaborator

xylar commented Dec 18, 2025

@cbegeman, this looks great!

  • I like the idea of a more general property check, of which the 3 conservation checks are a subset
  • I'm fine with verify_properties as argument to add_output_file, and as a list of string names of properties to check.
  • I'm good with verify_properties being a method of Step, overridden by OceanModelStep.
  • I think I do somewhat prefer check to verify so if you want to change that, I would be in favor. But I don't feel strongly about it.

Let me know when you're ready for a more detailed review.

@cbegeman
Copy link
Copy Markdown
Collaborator Author

@xylar Great. Thanks for taking a look! I'll try to finish this up today so we can get it in before the break

@cbegeman cbegeman self-assigned this Dec 18, 2025
@cbegeman cbegeman added enhancement New feature or request framework Changes relating to the polaris framework as opposed to individual tests or analysis ocean Related to the ocean component labels Dec 18, 2025
@cbegeman cbegeman force-pushed the add-conservation-checks branch from fca87df to 38a167d Compare December 18, 2025 20:20
@cbegeman cbegeman marked this pull request as ready for review December 18, 2025 20:21
@cbegeman
Copy link
Copy Markdown
Collaborator Author

cbegeman commented Dec 18, 2025

Testing

I have run the mass conservation check for the barotropic_gyre case for both MPAS-Ocean and Omega on chrys with intel, openmpi

@cbegeman cbegeman requested a review from xylar December 18, 2025 20:23
@cbegeman
Copy link
Copy Markdown
Collaborator Author

@xylar Let me know if you would like me to exclude 38a167d

@cbegeman cbegeman force-pushed the add-conservation-checks branch from 38a167d to 34b123b Compare December 18, 2025 20:24
@cbegeman cbegeman marked this pull request as draft December 18, 2025 20:31
@cbegeman cbegeman marked this pull request as ready for review December 18, 2025 22:48
@cbegeman
Copy link
Copy Markdown
Collaborator Author

@xylar Ok. It think this is now ready. I realized that it made sense to support conservation checks for all omega-ported tests.

@xylar
Copy link
Copy Markdown
Collaborator

xylar commented Dec 19, 2025

@xylar Let me know if you would like me to exclude 38a167d

Nope, I'm fine with sneaking those fixes in here.

Copy link
Copy Markdown
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

My main non-cosmetic comment is about what layerThickness means coming from Omega. This actually may be something we need to address in a separate PR. It may be that we need layerThickness <---> GeometricLayerThickness and we need to have a way to compute Omega's LayerThickness from GeometricLayerThickness as part of computing initial conditions. But I want us to have a plan for that because this PR will somewhat exacerbate that problem.

And, as I say below, I think we should strongly consider renaming LayerThickenss to PseudoThickness in Omega.

@cbegeman
Copy link
Copy Markdown
Collaborator Author

we need to have a way to compute Omega's LayerThickness from GeometricLayerThickness as part of computing initial conditions. But I want us to have a plan for that because this PR will somewhat exacerbate that problem.

@xylar Does this PR exacerbate that problem if we're calling separate functions for MPAS-O and Omega, e.g. compute_total_mass_nonboussinesq? I think you made a good point that we may want to use PseudoThickness rather than geometric layerThickness for Omega. But are you thinking that we would really want to convert to layerThickness for Omega for the tracer conservation checks?

@xylar
Copy link
Copy Markdown
Collaborator

xylar commented Dec 19, 2025

@xylar Does this PR exacerbate that problem if we're calling separate functions for MPAS-O and Omega, e.g. compute_total_mass_nonboussinesq? I think you made a good point that we may want to use PseudoThickness rather than geometric layerThickness for Omega. But are you thinking that we would really want to convert to layerThickness for Omega for the tracer conservation checks?

I think this mainly exacerbates the problem if we don't account for layerThickness = PseudoThickness. As long as we account for that, we're good. I'll need to fix things in #440 to make sure we don't use layerThickness there but rather PseudoThickness.

@cbegeman
Copy link
Copy Markdown
Collaborator Author

@xylar Do you want to take a look at the last 2 commits and let me know if I've made the changes you had in mind?

Copy link
Copy Markdown
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

@cbegeman, great! I think your solution to adding a helper function that takes care of all cases simultaneously quite clever!

Looks great to me!

@cbegeman
Copy link
Copy Markdown
Collaborator Author

@xylar Thanks for reviewing and getting started on some related draft PRs

@cbegeman cbegeman merged commit 3bcccb8 into E3SM-Project:main Dec 19, 2025
7 checks passed
@cbegeman cbegeman deleted the add-conservation-checks branch December 19, 2025 21:21
@xylar xylar mentioned this pull request Mar 10, 2026
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request framework Changes relating to the polaris framework as opposed to individual tests or analysis ocean Related to the ocean component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants