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

Update SphericalVector to work with StructuredColumns as source functionspace. #175

Merged
merged 7 commits into from
Mar 6, 2024

Conversation

odlomax
Copy link
Contributor

@odlomax odlomax commented Mar 4, 2024

This draft PR updates the test from PR #168 to include halo exchanges. Due to issue #174, the following code has to be added before Inderpolation::exectute is called:

  if (functionspace::StructuredColumns(sourceFunctionSpace)) {
    sourceFieldSet[0].metadata().set("type", "scalar");
    sourceFieldSet[0].haloExchange();
    sourceFieldSet[0].metadata().set("type", "vector");
  }

For other functionspaces, the type is set to vector and the halo exchange is called within Inderpolation::exectute.

If you comment out this block, you'll reproduce the output in #174 .

Once #174 is addressed, this block can be deleted and the PR will be ready for review.

Edit: further additions to this PR will close #174

@MarekWlasak
Copy link
Contributor

Is there a better way to sort this out? ... The code above looks a bit hacky.

@odlomax
Copy link
Contributor Author

odlomax commented Mar 4, 2024

Is there a better way to sort this out? ... The code above looks a bit hacky.

"Once #174 is addressed, this block can be deleted and the PR will be ready for review." 🙂

@odlomax
Copy link
Contributor Author

odlomax commented Mar 4, 2024

@wdeconinck @MarekWlasak I've commented on a very simple solution in #174 which I believe this PR closes.

I've also added some further tests to make sure sensible values are returned for points located on a pole.

Thanks, all!

@odlomax odlomax marked this pull request as ready for review March 4, 2024 17:36
@@ -3,7 +3,7 @@
*
* This software is licensed under the terms of the Apache Licence Version 2.0
* which can be obtained at http://www.apache.org/licenses/LICENSE-2.0.
* In applying this licence, ECMWF does not waive the privileges and immGeometryies
* In applying this licence, ECMWF does not waive the privileges and immunities
Copy link
Member

@wdeconinck wdeconinck Mar 5, 2024

Choose a reason for hiding this comment

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

Thanks for keeping our immunities intact 🙃

@@ -45,7 +43,10 @@ void GeometrySphere::xyz2lonlat(const Point3& xyz, Point2& lonlat) const {
/// @ref https://en.wikipedia.org/wiki/Great-circle_navigation
///
std::pair<double, double> greatCircleCourse(const Point2& lonLat1,
const Point2& lonLat2) {
const Point2& lonLat2) {
if (lonLat1 == lonLat2) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be using bool points_equal(const Point2&, const Point2&)

I have a more consistent way to compare these in an upcoming PR (a refactoring of the geometry library) and I've already included the "course" methods there, but they will need updating to your improved version, when that time comes :-)

Copy link
Member

Choose a reason for hiding this comment

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

That's correct, that it should use "points_equal". But that already happens. See https://github.com/ecmwf/atlas/blob/develop/src/atlas/util/Point.h#L32-L34.

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 97.27273% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 80.07%. Comparing base (4e2a055) to head (ffd61b3).

Files Patch % Lines
src/atlas/util/Geometry.cc 95.00% 2 Missing ⚠️
...terpolation/test_interpolation_spherical_vector.cc 98.14% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #175   +/-   ##
========================================
  Coverage    80.07%   80.07%           
========================================
  Files          859      859           
  Lines        63677    63686    +9     
========================================
+ Hits         50987    50997   +10     
+ Misses       12690    12689    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odlomax
Copy link
Contributor Author

odlomax commented Mar 5, 2024

@wdeconinck @pmaciel

Thanks for taking a look. Just to summarise, are there any changes you'd like? It looks like MacOS build is falling over, but I'm not sure if that's down to this PR.

@wdeconinck wdeconinck changed the title Update SphericalVector test to include halo exchange. Update SphericalVector to work with StructuredColumns as source functionspace. Mar 6, 2024
Copy link
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

Thanks @odlomax for the fixes and discussions.

@wdeconinck wdeconinck merged commit c3333e9 into ecmwf:develop Mar 6, 2024
20 checks passed
odlomax added a commit to JCSDA-internal/atlas that referenced this pull request Mar 6, 2024
wdeconinck pushed a commit that referenced this pull request Mar 7, 2024
wdeconinck added a commit that referenced this pull request Apr 9, 2024
* release/0.37.0: (23 commits)
  Update Changelog
  Version 0.37.0
  Projection base implementation derivatives performance/encapsulation … (#185)
  atlas_io is an adaptor library when eckit_codec is available (#181)
  Fix build for configuration setting ATLAS_BITS_LOCAL=64 (#184)
  Revert "Avoid linker warnings on macOS about 'ld: warning: could not create compact unwind for ...'"
  Cosmetic: readability braces
  Initialize std::array values to zero because valgrind complains, even though c++ standard mandates it should be default-initialized to zero
  Fix bug in TraceT caused by typo where the title was wrong
  Avoid linker warnings on macOS about 'ld: warning: could not create compact unwind for ...'
  Use new LocalConfiguration baseclass functions in util::Config and util::Metadata instead of eckit::Value backdoor
  Removed leftover code missed in PR #175
  Update `SphericalVector` to work with StructuredColumns as source functionspace. (#175)
  Bugfix for regional grids with ny > nx
  Refactoring of interpolation::method::SphericalVector and implementation of adjoint methods. (#168)
  Added test with empty integer sequence.
  Added arrayForEachDim method.
  Add docs build workflow
  Github Actions: Fix macOS MPI slots
  Fix for elements that might have unassigned partition via parallel Delaunay meshgenerator
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StructuredColumns FixupHaloForVectors method interferes with SphericalVector interpolation
5 participants