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

Feature/bump covariance #123

Merged
merged 48 commits into from
Nov 20, 2024
Merged

Feature/bump covariance #123

merged 48 commits into from
Nov 20, 2024

Conversation

frld
Copy link
Contributor

@frld frld commented Oct 18, 2024

Description

Add spatial covariances to the 3dvar and Dirac tests generated using BUMP.

Let there be light! We now have assimilation with spatial covariances.

Developing this change I did encounter an atlas error "Exception: Not implemented: Adjoint interpolation only works for interpolation schemes that are linear" after adding missing values to increments. This was because of the "non_linear: missing-if-all-missing" in the atlas interpolation. I understand that this may be worked on in future so for now I have added a QC filter to remove any observations affected by missing values.

I was considering updating the orca2 background field which has what I consider an incorrect land mask, but in this case it is quite useful as one of the observations is in the mask and it is a good test that my QC filtering is working. I will revisit this when I start looking at the NEMOVAR interface as this will require that the correct orca2 mask is used (or at least that NEMOVAR and orca-jedi agree).

Issue(s) addressed

N/A

Dependencies

List the other PRs that this PR is dependent on:
N/A

Impact

Expected impact on downstream repositories or workflows:
No impact

Checklist

  • I have updated the unit tests to cover the change
  • New functions are documented briefly via Doxygen comments in the code
  • I have linted my code using cpplint
  • I have run the unit tests
  • I have run mo-bundle to check integration with the rest of JEDI and run the unit tests under all environments

frld and others added 30 commits July 26, 2024 10:42
…t to

state method. Update yaml 3DVar yaml file so 3DVar runs through (but produces nans currently).
* Update ci container

* allow manual trigger of ci

* temporary change to force ci to run

* revert temporary change as ci now gets past cmake config step
creation of json schema again for 3dvar and errorcovariancetoolbox
applications.
@twsearle twsearle changed the title Feature/bump covariance [WIP] Feature/bump covariance Oct 23, 2024
@frld
Copy link
Contributor Author

frld commented Nov 8, 2024

Hi Toby,

This PR is now ready for review I think. The main thing this development does is add the ability to use BUMP to create background error correlations in the assimilation which means we can spread the observation information and the increment from any observation is now a blob rather than delta function.

I added a new ctest to construct the "NICAS" grid for BUMP. I tried to use settings to make this as quick as possible but note it takes 44 seconds to run ( a bit slower than the other tests ). I've also put in unit tests to test the new functionality I added.

All ctests pass.

The bbb test has passed see http://fcm1/cylc-review/taskjobs/frld?&suite=mo-bundle-orca-jedi-bump

The list of commits is a bit long because it was a branch of a branch which I rebased and it has listed the commit messages from the parent branch as well as the commits for the child. The file differences showing up in github look correct though so I presume it will merge correctly once approved.

Thanks, Dan

@frld frld self-assigned this Nov 8, 2024
@frld frld marked this pull request as ready for review November 8, 2024 10:01
@frld frld changed the title [WIP] Feature/bump covariance Feature/bump covariance Nov 8, 2024
Copy link
Collaborator

@twsearle twsearle left a comment

Choose a reason for hiding this comment

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

I gave this my best first pass of a review and it is looking good to me! I have a few comments to aid my understanding so that I can give a proper review, as well as a few questions about the interface to the atlas-orca grids that it would be good to nail down.

At some point I would quite like to request a review from someone working on land DA to get their view on the general layout and if they have any additional thoughts.

src/orca-jedi/geometry/Geometry.cc Outdated Show resolved Hide resolved
src/orca-jedi/geometry/Geometry.cc Show resolved Hide resolved
src/orca-jedi/geometry/Geometry.cc Outdated Show resolved Hide resolved
src/orca-jedi/geometry/Geometry.cc Outdated Show resolved Hide resolved
src/orca-jedi/geometry/Geometry.cc Show resolved Hide resolved
src/orca-jedi/geometry/Geometry.cc Outdated Show resolved Hide resolved
src/orca-jedi/geometry/Geometry.h Outdated Show resolved Hide resolved
src/orca-jedi/geometry/GeometryParameters.h Outdated Show resolved Hide resolved
src/tests/orca-jedi/test_state.cc Show resolved Hide resolved
src/tests/testinput/3dvar_sic.yaml Outdated Show resolved Hide resolved
@twsearle
Copy link
Collaborator

One additional thing that just occured to me - it would be great if you could make some small changes to the README (can be very light touch) just to point to the additional dependencies, and where to go to find out how to write the configuration for the interface to SABER (I assume this is in the jedi technical docs... hopefully).

@frld
Copy link
Contributor Author

frld commented Nov 13, 2024

One additional thing that just occured to me - it would be great if you could make some small changes to the README (can be very light touch) just to point to the additional dependencies, and where to go to find out how to write the configuration for the interface to SABER (I assume this is in the jedi technical docs... hopefully).

I've added SABER to the dependencies. SABER is (unusually for JEDI) well documented in the standard JEDI documentation here https://jointcenterforsatellitedataassimilation-jedi-docs.readthedocs-hosted.com/en/latest/inside/jedi-components/saber/index.html I've added a link to this to README too. Let me know if more is required. One question is there a way to see how the README.md would look in github before it goes back to develop?

custom routine to determine the i and j points.
@frld
Copy link
Contributor Author

frld commented Nov 13, 2024

Toby,
I've rerun the all the orca-jedi ctests after the updates I made in response to your review and everything still passes.
Cheers, Dan

Copy link
Collaborator

@twsearle twsearle left a comment

Choose a reason for hiding this comment

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

Great thanks for all your changes! Hopefully we can get a more performant/robust solution for the land-sea mask soon.

@twsearle twsearle merged commit 0ec97d1 into develop Nov 20, 2024
2 checks passed
@frld frld deleted the feature/bump_covariance branch November 20, 2024 17:15
@frld
Copy link
Contributor Author

frld commented Nov 20, 2024

Great thanks for all your changes! Hopefully we can get a more performant/robust solution for the land-sea mask soon.

Thanks. I agree about the land-sea mask but for now this sufficient for my next step...

Brace yourself the next step is the NEMOVAR interface! The interaction with orca-jedi is not too heavy due to the wonders of JEDI most interactions are through SABER. I did get this all working (again) without too much trouble but then I updated NEMOVAR and now I've got a job of work on... (See https://github.com/MetOffice/orca-jedi/tree/feature/nemovar_interface )

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.

2 participants