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

Projection base implementation derivatives performance/encapsulation … #185

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

pmaciel
Copy link
Member

@pmaciel pmaciel commented Mar 27, 2024

Projection base implementation derivatives performance/encapsulation improvements (better use of temporaries). This comes from the eckit::geo library.

Mesh adds a missing header atlas/runtime/Exception.h

@pmaciel pmaciel requested a review from wdeconinck March 27, 2024 07:57
@wdeconinck
Copy link
Member

Thanks @pmaciel for cleanup and possibly resulting performance improvements. What caused you to tackle this?
At first sight looks OK, but a number of tests are now failing with FE_DIV_BY_ZERO:
The following tests FAILED:
75 - atlas_test_grid_regional_lambert_conformal_conic_1 (Failed)
76 - atlas_test_grid_regional_lambert_conformal_conic_2 (Failed)
78 - atlas_test_grid_regional_mercator_1 (Failed)
79 - atlas_test_grid_regional_mercator_2 (Failed)
80 - atlas_test_grid_regional_mercator_3 (Failed)

@@ -49,8 +45,7 @@ class ProjectionImpl : public util::Object {
static const ProjectionImpl* create(const eckit::Parametrisation& p);
static const ProjectionImpl* create(const std::string& type, const eckit::Parametrisation& p);

ProjectionImpl() = default;
virtual ~ProjectionImpl() = default; // destructor should be virtual
Copy link
Member

Choose a reason for hiding this comment

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

Is there no need for a virtual destructor?

@pmaciel
Copy link
Member Author

pmaciel commented Mar 27, 2024

Thanks @pmaciel for cleanup and possibly resulting performance improvements. What caused you to tackle this? At first sight looks OK, but a number of tests are now failing with FE_DIV_BY_ZERO: The following tests FAILED: 75 - atlas_test_grid_regional_lambert_conformal_conic_1 (Failed) 76 - atlas_test_grid_regional_lambert_conformal_conic_2 (Failed) 78 - atlas_test_grid_regional_mercator_1 (Failed) 79 - atlas_test_grid_regional_mercator_2 (Failed) 80 - atlas_test_grid_regional_mercator_3 (Failed)

Good question; these are just a set of changes I had here when porting the bounding box calculation to eckit::geo (it is a requirement for earthkit.maps). the changes are only a cleanup/minor performace regarding avoiding so many temporaries of KPoint, and precompute recurring divisions -- which the bisection methods relies on. Functionality should be absolutely identical, so I'm surprised by the FPE issue being triggered (good catch, I have to cover for that for sure).

What's the platform/compiler combination for this triggering?

…es; MercatorProjection fix on handling extrema
@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2024

Codecov Report

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

Project coverage is 80.06%. Comparing base (7abbf1a) to head (3f2e234).

Files Patch % Lines
src/atlas/projection/detail/ProjectionImpl.cc 86.95% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #185      +/-   ##
===========================================
- Coverage    80.07%   80.06%   -0.01%     
===========================================
  Files          859      798      -61     
  Lines        63684    61943    -1741     
===========================================
- Hits         50995    49596    -1399     
+ Misses       12689    12347     -342     

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

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 seems to have fixed it!

@wdeconinck wdeconinck merged commit 270b936 into develop Apr 9, 2024
110 of 112 checks passed
@wdeconinck wdeconinck deleted the feature/projection-improvements branch April 9, 2024 14:45
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants