-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix sea surface temperature cycling #93
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix! Do you an idea of the constant SST is an issue in the frac_grid=True
case as well?
|
Oops I take this back...upon reading the code again, this code change would not be active in the See this note in the logs:
and this spot in the code: If I had to guess, I would say that relaxing SSTs back to the climatology is also broken in the by changing |
I suppose it's a little awkward in the Will revert back to 74c4015 for now. I think it's better to leave that possibly broken rather than have a potentially incorrect fix (I also can't test the I'm reasonably confident that in the |
Hey, yeah sorry didn't mean to send you down a rabbit hole. Was just curious if we at least knew if it would work in that case. Agreed it's not a priority to run with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you needed was there but not!!
Note that this does not necessarily fix the case when running with frac_grid = .true., but to date we have only run with frac_grid = .false.
This PR adds a test confirming that with the use of the namelist flag added in ai2cm/fv3gfs-fortran#173, setting the "ocean_surface_temperature" modifies the evolution of the model's prognostic variables (e.g. air temperature). In between ai2cm/fv3gfs-fortran#173 and ai2cm/fv3gfs-fortran#93, this functionality was broken (see the failing test associated with 5a6febb of this PR). In other words, since ai2cm/fv3gfs-fortran#93, Sfcprop%tsfco in the fortran model was modified within the physics driver prior to being used by any physics scheme; therefore when we set it via Python, it was immediately set to something else in the fortran before it could have any impact. The namelist parameter toggles the behavior back to what it was before ai2cm/fv3gfs-fortran#93, allowing us to prescribe the SST.
Prepare a release of fv3gfs-python. See HISTORY.md for more details about this release. Changes: - Update fv3gfs-fortran submodule to current master - Update fv3util sub-module to v0.5.1 - Cleanup bug fixes from moving fv3util to a separate submodule - Fixed bumperversion configuration - Avoid triggering fv3util tests (which were failing). This means that the gt4py numpy backend is currently not being tested anywhere.
This is the alternative fix mentioned in #92. This works without having to change the regression test checksums, which we might hope would be the case considering that the regression test only runs for 30 minutes and has a cycling frequency of 24 hours. In theory any change we make here shouldn't change answers, since the model isn't run long enough for cycling to be called.
Tests can be found in this notebook. EMC seems to think this fix is the correct approach.