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

refactor bkgerrstddev into a saber block #1085

Merged
merged 26 commits into from
Nov 18, 2024

Conversation

travissluka
Copy link
Collaborator

@travissluka travissluka commented Oct 22, 2024

Description

This PR converts the existing BkgErrGodas linear variable change into a ParametricOceanStdDev saber block (that lives in the SOCA repo for now), written in C++

jedi-ci-test-select=intel

Why?

  • This is the first step in generalizing the code enough to be moved out of the SOCA repo and into SABER. Similar to what was done with diffusion, I'll save moving it out of SOCA for a later PR (once we're sure it works as intended and is indeed generic)
  • I need to move this into a saber block if I want to move the balance operator into a saber block (which I do want to do)
  • It's slightly faster and uses slightly less memory, since we are no longer going in and out of the Fortran code (see timings below)
  • By default, this saber block uses the oops::Diffusion class to smooth the resulting stddev by the length scales. Much less noisy now (see images below), the scales by which the fields are smoothed can be configured. Unlike with the correlation operator, no offline parameters need to be calculated to use the smoothing (since there is no normalization)
  • By being in C++, now all the yaml parameters are checked when they are read in. No more accidentally setting wrong parameters that don't do anything!
  • the "other variable" section (eg. all variables other than T/S/SSH) was hardcoded in the Fortran. This removes that hardcoding and allows for a "fraction of background / min / max" to be set for any input variable

Also,

  • I fixed a bug where the values along the bottom level were too large (not sure how we missed that before), (see images below)

Usage:

The saber block has reasonable default values set (in an effort to keep outside collaborators from trying to do crazy things), so, for example you could use a minimum yaml such as

- saber block name: SOCAParametricOceanStdDev
  temperature:  # use defaults other than SST input file
    sst:
      filepath: data_static/godas_sst_bgerr.nc
      variable: sst_bgerr
  unbalanced salinity: {} # use default values
  unbalanced ssh: {} # use default values

this will still enable the smoothing by default.

Or, if you want to set parameters explicitly, see what is given in the testinput/dirac_parametric_ocean_stddev.yml file, or look at the parameters class in src/soca/SaberBlocks/ParametricOceanStdDev/ParametricOceanStdDev.h (real documentation will be coming, I promise!)

Testing

  • All ctests have been updated
  • a 1/4 deg test has been run. background error is less noisy, doesn't have the bug at the bottom, and the multiply is faster

This is the T bkg error at lvl 15 (very noisy
image

Same level but with this new saber block, where the stddev is smoothed by the Rossby radius
image

@travissluka travissluka added the SOCA Sea-ice, Ocean, and Coupled Assimilation label Oct 22, 2024
Copy link
Collaborator

@kbhargava kbhargava left a comment

Choose a reason for hiding this comment

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

🎉

@Dooruk
Copy link

Dooruk commented Oct 28, 2024

Nice, our group has been looking at this part of the code lately and we are coming up with some questions. Could we expand on the parametric part, for instance bumping SSS error around major river outputs? We could use a static "river map" input of sorts and this could be an optional setting.

One (obvious) thought we were pondering beyond this is to be able to use atmospheric fields, say cloud cover for large storms. Other than surface fluxes, can SOCA access/utilize any atmospheric fields? Is jcsda-internal/coupling the only way of doing that properly? If fv3-jedi is used at each cycle in a weakly-coupled sense, can any fields be utilized within the SOCA context?

Also, please keep the documentation effort going like you mention here. soca-tutorial has been super helpful 🙏

@travissluka
Copy link
Collaborator Author

Nice, our group has been looking at this part of the code lately and we are coming up with some questions. Could we expand on the parametric part, for instance bumping SSS error around major river outputs? We could use a static "river map" input of sorts and this could be an optional setting.

One (obvious) thought we were pondering beyond this is to be able to use atmospheric fields, say cloud cover for large storms. Other than surface fluxes, can SOCA access/utilize any atmospheric fields? Is jcsda-internal/coupling the only way of doing that properly? If fv3-jedi is used at each cycle in a weakly-coupled sense, can any fields be utilized within the SOCA context?

Also, please keep the documentation effort going like you mention here. soca-tutorial has been super helpful 🙏

@Dooruk, yes. The easiest and most flexible thing for me is to make the SSS error read from a file like the SST error is. I can add that capability in a follow-up PR. We could read in atmospheric fields, but that would require you to interpolate them onto the MOM grid offline, then add them to the MOM state.

@travissluka travissluka mentioned this pull request Oct 30, 2024
@travissluka
Copy link
Collaborator Author

This is ready for merge. Does anyone else have time for a second review? (@guillaumevernieres @Dooruk ?)

Copy link
Contributor

@HamidehGMAO HamidehGMAO left a comment

Choose a reason for hiding this comment

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

It looks good to me! and it builds fine and all the tests pass except the test 39 for ref. mismatch reasons. I do not see it failing in travis-CI. Anyway it isn't related to this PR.

@travissluka
Copy link
Collaborator Author

It looks good to me! and it builds fine and all the tests pass except the test 39 for ref. mismatch reasons. I do not see it failing in travis-CI. Anyway it isn't related to this PR.

@HamidehGMAO running on Discover I'm assuming? Intel or GCC? It would be nice to update the reference answers at some point.

@HamidehGMAO
Copy link
Contributor

yeah! running on discover and using intel, spack stack 1.7

@travissluka travissluka merged commit fe20d58 into develop Nov 18, 2024
2 checks passed
@travissluka travissluka deleted the feature/refactor_bkgerrstddev branch November 18, 2024 20:07
@Dooruk
Copy link

Dooruk commented Dec 2, 2024

ignore this comment

I'm now updating our workflow for the sprint name changes + this particular PR.

@Dooruk
Copy link

Dooruk commented Dec 2, 2024

Well, I just tested 5-deg within the Swell context rather than my sandbox and it worked there.. so it was just a fluke. I will test with 0.25-deg next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SOCA Sea-ice, Ocean, and Coupled Assimilation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

move BkgErrGodas to a saber outer block BkgErrorGODAS needs a QC step
4 participants