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

Add increment code needed for 3DVar #88

Merged
merged 4 commits into from
May 7, 2024
Merged

Add increment code needed for 3DVar #88

merged 4 commits into from
May 7, 2024

Conversation

frld
Copy link
Contributor

@frld frld commented May 3, 2024

Description

This adds some of the increments class methods needed for data assimilation (DA) to orca-jedi and updates the increments unit test to test this. **NB this is continuation of PR #85

This PR doesn't including all the increments methods needed for DA in particular I left out the (netcdf) writing code, I'll put this in a later PR. I have also attempted to minimise the changes to other parts of the code in this PR. I'm hoping this means it is not too big to review.

FYI my draft list for PRs below

  • Changes to increment (this PR)
  • Changes to state, nemo field writer/reader
  • Changes to interpolator / add dirac test (delta function test)
  • Implement Saber / error covariance training including changes to geometry needed for this.
  • 3dvar executable/test
  • +++ Multiprocessing / Nemovar / Multiple variables / Linear balance / orca025ext / mpi ....

Currently this code only works for single processor runs. I was hoping to leave implementing multiprocessing for a future PR.

Also I am aware that both the increments methods and state methods operate on atlas fieldsets so there is the possibility of calling generic methods for both increment and state using a generic fields class. I note this approach is used by lfric and also oops/l95 and probably other code in JEDI. Let me know if you have any thoughts on this. It might be nice to do this in orcajedi too, but ideally not this PR. I think it would be helpful to get the increment test (along with my initial code) in place first.

Issue(s) addressed

N/A

Dependencies

N/A

Impact

There is no impact on other code as far I am aware.

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've run the coding_norms test and it passes)
  • I have run the unit tests (all the orca-jedi ctests have run and passed)
  • I have run mo-bundle to check integration with the rest of JEDI and run the unit tests under all environments

frld added 2 commits May 3, 2024 17:54
setup most increments class methods needed for DA and add unit tests for
them.
@frld frld self-assigned this May 3, 2024
@frld frld mentioned this pull request May 3, 2024
@frld
Copy link
Contributor Author

frld commented May 3, 2024

A new mo-bundle test is now running see http://fcm1/cylc-review/cycles/frld/?suite=mo-bundle-orca-jedi-increment3

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.

Its all looking good, just a couple of discussion points, and a question.

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.

sorry spotted one more thing

@@ -105,6 +105,8 @@ class State : public util::Printable,
const oops::Variables & variables() const {return vars_;}
oops::Variables & variables() {return vars_;}

atlas::Field getField(int) const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I just realised one more thing - this one won't be tested. Could you please add a test to src/tests/orca-jedi/test_state.cc just to retrieve a field and check its name matches what is expected? I can help with this if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll give it a go and let you know if I'm stumped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a basic test. Does it fit the bill?

@frld
Copy link
Contributor Author

frld commented May 7, 2024

A new mo-bundle test is now running see http://fcm1/cylc-review/cycles/frld/?suite=mo-bundle-orca-jedi-increment3

These all passed except for a few lfric tests which run out of wallclock. This is a known issue unrelated to my developments.

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.

Looks good and passes the tests, thanks for the contribution!

@twsearle twsearle merged commit 5b00a57 into develop May 7, 2024
2 checks passed
@frld frld deleted the feature/increment branch May 7, 2024 15:39
@frld frld mentioned this pull request May 8, 2024
5 tasks
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