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

Initial stochastic physics implementation #48

Closed
wants to merge 36 commits into from

Conversation

pjpegion
Copy link

This PR contains the initial implementation of 2 ocean stochastic schemes (SPPT and Energetic PBL perturbations). I have bracketed the stochastic physics with pre-processor directives so it will not branch GFDL and NCAR's use of the model.

There will be associated PRs in fv3atm, ccpp_physics, that include additional changes not associated with the ocean and
stochastic_physics which is necessary for the ocean stochastic physics.

@pjpegion
Copy link
Author

This references issue #52

@jiandewang
Copy link
Collaborator

@pjpegion: I am having hard time to find the modules you add in your branch:
"use stochastic_physics", "use get_stochy_pattern_mod". Where do these modules exist?

@pjpegion
Copy link
Author

@jiandewang those subroutines are in the stochastic_physics repo: PR is
https://github.com/noaa-psd/stochastic_physics/pull/35/files

@JessicaMeixner-NOAA
Copy link
Collaborator

So there is this conversation here: https://github.com/NOAA-GFDL/MOM6/discussions/1286 about MOM6 code restructuring. I'm wondering if the solution to getting rid of some of the "if defs" would be to have the parts of the code that actually call the external stochastic physics packages go into the "external" folder?

Also if the if defs remain... shouldn't this be like "if stochastic physics"? I can see even the UFS being run w/out stochastic physics?

@jiandewang
Copy link
Collaborator

@pjpegion got your point, let's leave it as is now.

@pjpegion
Copy link
Author

pjpegion commented Feb 2, 2021

Sounds good, I'm willing to work with NCAR or GFDL if that would like to add this functionality to their versions of the model.

@jiandewang
Copy link
Collaborator

@pjpegion I think eventually the stochastic part will be added in mct, and we can coordinate with NCAR on testing.
@DeniseWorthen @JessicaMeixner-NOAA: I talked to Bob on this PR yesterday on regular MOM6 meeting, GFDL will be involved in code review.

src/core/MOM.F90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@DeniseWorthen DeniseWorthen left a comment

Choose a reason for hiding this comment

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

Have you been able to compile and run this code in the ufs-weather-model w/o the ocean stoch active?

@pjpegion
Copy link
Author

pjpegion commented Feb 4, 2021 via email

@DeniseWorthen
Copy link
Collaborator

DeniseWorthen commented Feb 4, 2021

You had said that you were able to run the cpld and datm tests so I was wondering where your MOM_input changes were added. I understand your point but we won't actually update MOM6 until ufs-weather-model PR is ready to merge (all RTs are run and pass) which means they'll need to be part of the ufs-weather-model PR.

Related to the param settings, which (if either), of the two settings can be independently set? Can stoch be run w/ only one of the two? Or are they orthogonal---if one is set the other isn't? Depending on the logic, would having a single "do ocean stoch" flag be useful (with two different ways of doing it)? I'm just not clear enough on how it all fits together.

@pjpegion
Copy link
Author

pjpegion commented Feb 4, 2021

The cpld and datm tests are without stochastic physics, so nothing changed in the MOM_input file since the default for do_sppt and pert_epbl. I still need to add a regression test that does test the ocean stochastic physics, just have not got there yet.
RE the parameter settings, they are independent of each other, so an additional do_ocean_stoch would work. It would clean up the if statements but I think make the configure file, work flow messier since there is one more namelist setting that has to be set.

@jiandewang
Copy link
Collaborator

jiandewang commented Feb 4, 2021

these are from Bob of GFDL and Phil's reply (I put them here to keep the conversions for record):

Bob: I have taken a look at Phil Pegion's PR, and it strikes me that there are a few things that I would do differently, but exactly what we would want to do probably depends on what other changes are intended to follow this, and where in the code those other changes might be experienced.

As written this PR is adding 3 2-d arrays that are modulating the ustar forcing fields as used in the ePBL boundary layer scheme. So my inclination would not be to add a whole other stochastic type that is passed down as a separate argument through multiple levels of the code, but I would add the required arrays as allocatable arrays in the same forcing type where the ustar fields are. The coupler sets up the fields in the forcing type, so the calls to the stochastic drivers could come in that same part of the NUOPC coupler that sets up other forcing fields. For the other couplers, there is no problem with there being an allocatable element to the forcing type that they simply don't touch. However, if this is ultimately intended to be deployed much more widely, and in places where the forcing type is not present, we might need to reconsider this strategy. Apart from this, we just ask that the conventions regarding line-length and spacing, and perhaps how to handle nested do-loops might be advisable, so as to match the new code with the existing style, if possible.

How extensive do you ultimately expect all of the changes to be?

Phil: I intentionally created a stochastic_physics derived type because this is just the initial implementation, so I think there will be more random patterns added over time. I originally considered putting them in the forcing container, but thought it wasn't appropriate since it is not really a forcing. I will have to think about if the patterns are needed in places where the forcing container is not accessible, and see if I could just add them there.

Hi Phil,

For a situation like this, we have sometimes used blank wrappers of the interfaces that we would call in a directory under MOM6/config_src/external, and in some cases the actual version might be included via a link under MOM6/pkg, although the latter is not necessary and probably is not the right answer in this case. The selection is made at compile time by selecting which directories are included in the build.

The both the blank version that a user gets when they checkout MOM6 and the external package have to have matching interfaces (or at least the bits called from with the MOM6 code have to be compatible - the real version can have extra optional arguments, for example), which is the one point of potential fragility to this approach, but so-far it seems to be working. For a concrete example see https://github.com/NOAA-GFDL/MOM6/tree/dev/gfdl/config_src/external/ODA_hooks, which deals with exactly this issue for data assimilation calls.

Bob,
I will have to take a look and see how I can adapt the stochastic physics to work like this. Not sure if I will have it ready by this PR, but will hopefully have something in the not too distant future.
Thanks,
Phil

@pjpegion
Copy link
Author

pjpegion commented Feb 5, 2021

Have you been able to compile and run this code in the ufs-weather-model w/o the ocean stoch active?

Yes, the non-stochastic physics coupled and data atmosphere regression tests pass.

@@ -453,7 +503,6 @@ subroutine diabatic_ALE_legacy(u, v, h, tv, Hml, fluxes, visc, ADp, CDp, dt, Tim
!! unused have NULL ptrs
real, dimension(:,:), pointer :: Hml !< Active mixed layer depth [Z ~> m]
type(forcing), intent(inout) :: fluxes !< points to forcing fields
!! unused fields have NULL ptrs
type(vertvisc_type), intent(inout) :: visc !< vertical viscosities, BBL properies, and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Accidental removal of comment line continuation

@DeniseWorthen
Copy link
Collaborator

I think this is looking pretty good now. My question now is more structural (Bob's comment) and whether that is the form they'd prefer it in to get it back to GFDL.

jiandewang pushed a commit to jiandewang/MOM6 that referenced this pull request Jun 17, 2021
merge in latest dev/gfdl updates
@pjpegion
Copy link
Author

I'm going to close the PR and open a new one with many of these issues addressed.

@pjpegion pjpegion closed this Jul 26, 2021
jiandewang pushed a commit to jiandewang/MOM6 that referenced this pull request Feb 1, 2022
…MC#48)

* Refresh attempt to get rid of NetCDF calls

* Fix comments

* Set netCDF attrs in MOM_horizontal_regridding

This patch sets the following netCDF attributes in the function
`horiz_interp_and_extrap_tracer_record` via `read_attribute`.

* `missing_value` (as `_FillValue`)
* `scale_factor`
* `add_offset`

This resolves some issues related to uninitialized values.

* read_variable_2d in horizontal remapping

This patch extends the `read_variable` interface to include 2d array
support, in order to facilitate domainless I/O via netCDF calls.

This is far from the best implementation (e.g. read_variable_2d
introduces another `broadcast` alongside the original one in the
horizontal regridding) but it addresses the immediate issues with
`MOM_read_data()`.

* set default scale factor to 1

* add missing start/count arguments

* Update MOM_io.F90

* Manage optional args in read_variable_2d

This patch modifies read_variable_2d so that the size() tests of the
optional arguments are applied before the call to nf90_get_var.

The tests are also wrapped inside present() flags to avoid checking
unassigned variables.

Thanks to Robert Hallberg for the suggestions.

Co-authored-by: Matthew Harrison <Matthew.Harrison@noaa.gov>
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.

4 participants