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 #85

Closed
wants to merge 6 commits into from
Closed

Add increment code #85

wants to merge 6 commits into from

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.

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

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 your 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

  • [ x ] I have updated the unit tests to cover the change
  • [ ] New functions are documented briefly via Doxygen comments in the code (I've attempted to follow the existing style which doesn't seem to have comments of this type)
  • [ x ] I have linted my code using cpplint (I've run the coding_norms test and it passes)
  • [ x ] 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 frld requested a review from twsearle May 3, 2024 11:28
@frld frld self-assigned this May 3, 2024
@twsearle
Copy link
Collaborator

twsearle commented May 3, 2024

Hi Dan, the doxygen comments are something we are adding as we make changes to the code, so it is in some places but not everywhere. For examples see OOPS or this recent change for example:

/// \brief Read in a 2D horizontal slice of variable data on the root processor only

It is good to have this stuff in as it can be unclear why a method has been added, what need it fulfills. However having said that I would expect Increments method descriptions to be very straight forward.

I am not sure about the ci issue, I am looking into it. In the mean time, could you please provide a cylc review link to a run of mo-bundle? To run the build and test inside a workflow you can use bin/bbb ctest <workflow-name>.


for (atlas::Field field : incrementFields_) {
std::string fieldName = field.name();
std::tie(valid_points, sumx2, sumx, min, max) = stats(fieldName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am thinking that this is a large number of ordered parameters - its easy to make a mistake and create a hidden runtime bug. I am thinking it would be clearer if stats returned a struct with these attributes instead of a tuple? Alternatively, maybe if stats is only used here you could define it as a lambda function local to Increment::norm?

@frld
Copy link
Contributor Author

frld commented May 3, 2024

I am not sure about the ci issue, I am looking into it. In the mean time, could you please provide a cylc review link to a run of mo-bundle? To run the build and test inside a workflow you can use bin/bbb ctest <workflow-name>.

Thanks. I've set a bbb ctest going see http://fcm1/cylc-review/cycles/frld/?suite=mo-bundle-orca-jedi-increment

A couple of GNU build task have already failed:

"CMake Error at /home/h03/darth/opt/mobbs-50/util/spice_gnu/share/ecbuild/cmake/ecbuild_log.cmake:190 (message):
CRITICAL - Feature MKL cannot be enabled -- following condition was
not met: MKL_FOUND"

Not sure what this means. It's the first time I've run bbb so it may well be user error. I did the following

module load bb-env
cd /data/users/frld/jedi/3dvar/mo-bundle (my bundle with my orca-jedi branch and with CMakeList updated to point to it)
WORKFLOW_NAME=mo-bundle-orca-jedi-increment
./bin/bbb ctest "${WORKFLOW_NAME}"

This triggers a cylc 7 suite (is this correct?)

@twsearle
Copy link
Collaborator

twsearle commented May 3, 2024

That ought to be correct. For more details you can check docs here:
https://github.com/MetOffice/mo-bundle?tab=readme-ov-file#rose-stem-workflow-usage

Have you introduced any new dependencies? I am not sure why you would see an MKL error. Either it is there on develop or something to do adding saber would be my guess?

@frld
Copy link
Contributor Author

frld commented May 3, 2024

Ah yes it could be. I forgot I was adding nemovar to the bundle. I don't need that problem right now! I've left the other workflow going for interest as it seems to be partially working and started a new one

http://fcm1/cylc-review/cycles/frld/?suite=mo-bundle-orca-jedi-increment2

Edit: Yes that was the issue. Removing the nemovar repo gets rid of that MKL error.

BTW thanks for taking the time to help with these issues. Hopefully everything will be a lot smoother for the next pull request.

comments. Also added unit tests for increments to and from fieldset.
@frld frld mentioned this pull request May 3, 2024
5 tasks
@frld
Copy link
Contributor Author

frld commented May 3, 2024

Closing this PR as I've moved the work to a direct branch from MetOffice/orca-jedi. See PR #88 for the continuation.

@frld frld closed this May 3, 2024
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