Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding the gradient model into MOM6 #755

Open
wants to merge 24 commits into
base: dev/gfdl
Choose a base branch
from

Conversation

sinakhani
Copy link

This is an implementation for subgrid ocean mesoscale eddy transport for running ocean simulations at eddy-permitting and non-eddying resolutions. The gradient model is introduced in Khani & Dawson (2023) [https://agupubs.onlinelibrary.wiley.com/doi/full/10.1029/2022MS003356]. The model is added into MOM_lateral_mixing_coeffs.F90 and MOM_thickness_diffuse.F90 files.

I am ready to work with you on integrating this model into MOM6. Thank you for your consideration.

@marshallward
Copy link
Member

@sinakhani Can you direct this to the dev/gfdl branch rather than the main branch?

@sinakhani sinakhani changed the base branch from main to dev/gfdl November 6, 2024 18:41
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.

While I appreciate this effort to add a new parameterization to MOM6, this PR has numerous problems that absolutely need to be corrected before it can be further considered for incorporation into the shared MOM6 code base.

  1. As written, this PR will change answers for cases that are not using the gradient model.
  2. The choice of calc_slope_functions_using_just_e() is at odds with the intended use of that function as a simple calculation of the slopes based only on the model interface heights.
  3. There are several new blocks of code with a comment like ! This branch is not used. If they are not being used, why introduce them?
  4. There are instances where the changes in units will reintroduce the use of the Boussinesq reference density into non-Boussinesq cases.
  5. There are instances that re-introduce the use of the obsoleted REMAPPING_2018_ANSWERS flag, which strongly suggests that when these code changes were merged with the most recent version of the code, it was done carelessly and errors were introduced.
  6. There are several places he the units of real variables (including arguments) are omitted or specified incorrectly.

After these changes have been corrected, I will be happy to reexamine these changes.

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