Skip to content

Use the generic oops interpolator#898

Merged
travissluka merged 10 commits intodevelopfrom
feature/soca_new_interp
Jun 15, 2023
Merged

Use the generic oops interpolator#898
travissluka merged 10 commits intodevelopfrom
feature/soca_new_interp

Conversation

@travissluka
Copy link
Contributor

@travissluka travissluka commented Jun 13, 2023

Description

Deletes the custom SOCA interpolation and uses the generic OOPS interpolation that @fmahebert put together, resulting in 768 fewer lines of SOCA code 🎉

In order to use the OOPS interpolation I had to fix the way halo regions were handled when being passed around atlas fieldsets

  • Previously, the halo could contain points that were duplicated on the PE, either in the halo elsewhere or in the non-halo region. This wouldn't work with the OOPS interpolation because it can't handle duplicate points. I now construct an array geom%valid_halo_mask that is true for the non-halo gridpoints and for the non-duplicate halo gridpoints (see soca_geom_find_invalid_halo()). Anytime a field is packed for atlas fieldset usage this mask is used.
  • It turns out we have invalid halo points being used and didn't even know it! (Not sure if this ever caused any real problems 🤷‍♂️ ). It seems that a small handful of halo points are not updated by the fms halo update calls, because they aren't used by mom6. I now set the geom lat/lon to INVALID_HALO (-999.0) instead of 0.0 before filling it with the correct lat/lon in the compute domain and doing the first halo update. I then test in soca_geom_find_invalid_halo() to see where there are still invalid lat/lon in the halo then set geom%valid_halo_mask to false for those gridpoints.

The one limitation, for now, is that all interpolation uses the same grid. Meaning U/V are assumed to be on the h grid. Options (in a subsequent PR) are

  1. give up the desire to have interpolation work on the native U/V grid. We don't even have u/v observations! We'll just make sure u/v are destaggered before running hofx
  2. I can maybe add back in some logic to have multiple oops interpolators constructed so that we can interpolate on the native u/v grid. I don't like this option, because it puts back in more soca-specific code.

We don't have to decide now. Just keep in mind if/when it is ever realistic that we'll have u/v observations.

Testing

  • Answers are different, as one would expect from changing the interpolation. On the plus side, the minimum coolskin temp is no longer -69 for hofx3d, but now a more realistic -4. Also, the interpolated sea ice concentration no longer goes above 1.0

  • The getvalues test has been mostly disabled (the masked fields were turned off). This is because of a limitation in the unit test. I need to talk to @fmahebert sometime to see how to fix this. (In short, I can't create proper soca analytic init values for the state and geovals because I never know for sure if a point near land will give a missing value or an interpolated value, it depends randomly on how the triangulation is performed for the oops interpolation) I'll deal with that "later"

  • There is a "bug" with LETKF not being able to handle missing values the same way the var does. The workaround for this is to make sure the landmark qc filter is in place for all letkf obs (which they should have been anyway)

  • Once I'm sure the CI tests are passing I'll run comparisons with a realistic domain and post some plots**

Dependencies

@travissluka travissluka added the waiting for another PR waiting on another pull request to be merged first label Jun 13, 2023
@travissluka travissluka self-assigned this Jun 13, 2023
@travissluka travissluka marked this pull request as ready for review June 13, 2023 19:58
Copy link
Contributor

@guillaumevernieres guillaumevernieres left a comment

Choose a reason for hiding this comment

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

🎉

@travissluka
Copy link
Contributor Author

note, I forgot to update 2 marine operators, I have updated the matching UFO PR. To make sure I have them all covered now, I updated the input obs files here to contain a single point at lat/lon 40.035, -105.243 which should definitely be land, otherwise the JCSDA building would be underwater.

Copy link
Contributor

@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.

Builds fine and ctests pass.

@travissluka
Copy link
Contributor Author

Looking at SST increment with the old code (left) and new interpolation (right)
image

There are small difference that seem reasonable given the big change in interpolation method. (new - old)
image

@travissluka
Copy link
Contributor Author

And looking at sst O-B for the old code (top) and new code (bottom) the "land ocean" points are now properly not having an hofx calculated. This should make diagnostics a lot easier going forward!

3dvar_old seaSurfaceTemperature OmB rmsd

3dvar seaSurfaceTemperature OmB rmsd

@kbhargava
Copy link
Contributor

Looking at SST increment with the old code (left) and new interpolation (right) image

There are small difference that seem reasonable given the big change in interpolation method. (new - old) image

Nice!!!

@travissluka travissluka merged commit 4231ce2 into develop Jun 15, 2023
@travissluka travissluka deleted the feature/soca_new_interp branch June 15, 2023 18:12
climbfuji added a commit that referenced this pull request Jul 17, 2023
* Use SABER blocks chaining for training (#852)

* Adapt to name changes in ufo (#867)

* Use VertInterp instead of MarineVertInterp (#874)

* remove linear var change from ensevariance ctest (#881)

* Bugfixes for BUMP (#869)

* A few fixes to the Model to Analysis variable change (#847)

* Remove discontinuities in the balance op  (#885)

* set to min/max instead of 0

* ...

* vanilla vert conv

* tidy sign

* removed jac_mask

* tanh filt

* updated yamls and test refs

* merge develop and update answers after the develop history MESS i created

* code tidy/removed vanilla

---------

Co-authored-by: Travis Sluka <tsluka@ucar.edu>

* default plots (#888)

Co-authored-by: Travis Sluka <tsluka@ucar.edu>

* add VADER to SOCA (#887)

* add vader to build, but dont use it

* add State::fromFieldSet

* variablechange "uses" vader

* switched to ens. templ. (#894)

* Implementation of EnsMeanAndVariance (#892)

* added the ensmeanandvariance app

* ...

---------

Co-authored-by: Travis Sluka <tsluka@ucar.edu>

* Read the std. dev. instead of the variance (#896)

* skip sqrt, saving optional

* forgot test yaml

* Update test/testinput/varchange_bkgerrsoca_stddev.yml

Co-authored-by: Travis Sluka <tsluka@ucar.edu>

---------

Co-authored-by: Travis Sluka <tsluka@ucar.edu>

* Use the generic oops interpolator (#898)

* use oops interpolation

* to_fieldset_ad

* forgot some halo masks

* fix horizfilt

* fix some test answers

* letkf workaround for missing val bug

* work around for getvalues test

* change obs input to have 1 guaranteed land point

* Update version (#899)

* remove ewok repo (#889)

Co-authored-by: Yannick Trémolet <30638944+ytremolet@users.noreply.github.com>

* Merge B matrix-related applications (#890)

* Merge B matrix-related applications

* Trigger tests

---------

Co-authored-by: Benjamin Menetrier <benjamin.menetrier@irit.fr>
Co-authored-by: Travis Sluka <tsluka@ucar.edu>

* update for VADER PR#586 (#902)

* add ModelData

* fix coding norms

* Update CMakeLists.txt

* Fix vertical gradient (#914)

* a fix dvdz

* Updating test references

---------

Co-authored-by: Hyun-Chul.Lee <Hyun-Chul.Lee@noaa.gov>

* libs in soca

* Commend out dirac.x test and disable ecbuild_install_project as we did for fv3-jedi prevent the following error:

-- Configuring done
CMake Error in soca/src/soca/CMakeLists.txt:
  Target "soca" INTERFACE_LINK_DIRECTORIES property contains path:

    "/Users/heinzell/work/ufs-bundle/20230714/ufs-bundle-use-jedi-develop/soca/src/soca/."

  which is prefixed in the source directory.

-- Generating done
CMake Generate step failed.  Build files cannot be regenerated correctly.

* Apply same bug fix as for fv3-jedi to fix missing linker path for MPI libraries due to ESMF bug

---------

Co-authored-by: Benjamin Menetrier <benjamin.menetrier@irit.fr>
Co-authored-by: Wojciech Śmigaj <w.smigaj@gmail.com>
Co-authored-by: Travis Sluka <tsluka@ucar.edu>
Co-authored-by: Guillaume Vernieres <guillaume.vernieres@noaa.gov>
Co-authored-by: Yannick Trémolet <30638944+ytremolet@users.noreply.github.com>
Co-authored-by: Benjamin Menetrier <30638301+benjaminmenetrier@users.noreply.github.com>
Co-authored-by: Kriti Bhargava <kritib@ucar.edu>
Co-authored-by: Hyun-Chul.Lee <Hyun-Chul.Lee@noaa.gov>
Co-authored-by: Dom Heinzeller <dom.heinzeller@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting for another PR waiting on another pull request to be merged first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Have interpolation return missing values on land

3 participants