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/add 1ob test #92

Merged
merged 8 commits into from
May 28, 2024
Merged

Feature/add 1ob test #92

merged 8 commits into from
May 28, 2024

Conversation

twsearle
Copy link
Collaborator

@twsearle twsearle commented May 24, 2024

Description

Add ctests to cover the eORCA025 mesh and alter one of the observations so that it is on the boarder between the two tasks when MPI=2. This gives a failing test until we merge developments for atlas-orca so that we can add halos.

Issue(s) addressed

Resolves #77
Part of #53

Dependencies

We should add the test without depending on the atlas orca change or any other change.

Impact

Expected impact on downstream repositories or workflows:
None

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

@twsearle twsearle self-assigned this May 24, 2024
@twsearle twsearle requested a review from s-good May 24, 2024 16:54
@twsearle
Copy link
Collaborator Author

@s-good this PR contains a test of the behaviour of the checkerboard partitioner. Once fixed, we ought to be able to remove the set_property line in CMakeLists and have the tests all pass.

@twsearle twsearle force-pushed the feature/add-1ob-test branch from 1b25003 to b27a5f7 Compare May 24, 2024 18:05
Copy link
Collaborator

@s-good s-good left a comment

Choose a reason for hiding this comment

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

Looks great, thanks.

if ( ( (params_.partitioner.value() == "serial") || (comm.size() == 1) )
&& (halo > 0) ) {
halo = 0;
params_.sourceMeshHalo.set(0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line doesn't work. I'm not sure what it should be, though.

@DJDavies2
Copy link
Contributor

This doesn't build for me:

/home/h01/frwd/cylc-run/orca-jedi-92/share/mo-bundle/orca-jedi/src/orca-jedi/geometry/Geometry.cc: In constructor ‘orcamodel::Geometry::Geometry(const eckit::Configuration&, const eckit::mpi::Comm&)’:
/home/h01/frwd/cylc-run/orca-jedi-92/share/mo-bundle/orca-jedi/src/orca-jedi/geometry/Geometry.cc:57:30: error: ‘class oops::Parameter’ has no member named ‘set’
57 | params_.sourceMeshHalo.set(0);

@twsearle
Copy link
Collaborator Author

apologies both, I was using the CI to test the build as I could not get an interactive build to work on my local machine for this change.

@twsearle twsearle requested a review from DJDavies2 May 28, 2024 08:56
@twsearle twsearle merged commit 748ac3d into develop May 28, 2024
2 checks passed
@twsearle twsearle deleted the feature/add-1ob-test branch June 10, 2024 07:46
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.

Add test to cover issues with higher resolution grids, such as ORCA025
3 participants