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

Add maximum number of iterations in find_depth_of_pressure_in_cell #609

Merged

Conversation

claireyung
Copy link

Adds a maximum number of iterations to the subroutine find_depth_of_pressure_in_cell in MOM_density_integrals

This subroutine is called in MOM_state_initialization and calculates the pressure within a cell to determine where the ice shelf should be positioned vertically. It takes a z tolerance (controlled by the runtime parameter TRIM_IC_Z_TOLERANCE) and then iterates until the pressure anomaly from the reconstruction in the cell to the target pressure is sufficiently small (controlled by pa_tol = g x rho x z_tol).

Currently, the model just has a while loop for the tolerance, which means if the TRIM_IC_Z_TOLERANCE is set to a very small number e.g.1e-15, and convergence cannot be achieved due to numerical precision, then the model will hang and continue looping forever.

I added a counter to count how many times it has iterated. In my preliminary tests with linear stratification for a few different geometries (therefore sampling a few different types of cells), depending on TRIM_IC_Z_TOLERANCE the subroutine either converged within 5 iterations and exited, or did not converge and kept repeating and hanging. Therefore I have hardcoded 30 as a maximum number of iterations. I currently have the model giving a fatal error if you exceed this (this would change answers from the model hanging and never running to the model giving a fatal error. Thus, this PR should not affect ice shelf initialisations that converged, assuming they also did so within 30 iterations, and should not change answers for any other configurations (note find_depth_of_pressure_in_cell is also called as a test in MOM_state_initialization but I don't see it used anywhere else.) Happy to add the bugfix as a runtime parameter if preferred!

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 very much for this contribution. It is a valuable improvement to the model behavior. In this case, I think that the hard-code limit of 30 iterations should be more than enough to trap pathological behavior but avoid failing when things might eventually converge. At some later date, we might consider adding guidance on what a user might do to fix this problem (e.g., the parameter to change to set a less stringent tolerance), but for now I think that should accept this contribution as it stands.

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.

I added some minor comments on the formatting of strings and comments. These are very low priority.

src/core/MOM_density_integrals.F90 Outdated Show resolved Hide resolved
src/core/MOM_density_integrals.F90 Outdated Show resolved Hide resolved
src/core/MOM_density_integrals.F90 Outdated Show resolved Hide resolved
@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/23554 ✔️

@marshallward marshallward merged commit 9f3da0b into NOAA-GFDL:dev/gfdl May 21, 2024
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