upgrade the halo exchange procedure for the function space 'PointCloud' #120

mo-lormi · 2023-03-28T15:38:53Z

Description

This PR is intended to upgrade the procedure for the halo exchange for the function space PointCloud, in particular to:

implement a new procedure to evaluate the fields partition and remote_index to be used for the halo exchange
add new tests to the test battery

Acceptance Criteria (Definition of Done)

All the tests pass.

…ote_index' to be used for the halo exchange

FussyDuck · 2023-03-28T15:38:59Z

All committers have signed the CLA.

mo-lormi · 2023-03-29T10:31:08Z

notes

The procedure is valid under the assumption that the spatial locations (Lat/Lon) are unique in a given 2D domain. In particular, the procedure relies on the fact that the locations of the 'own points' are unique within each partition.
The procedure can process data associated with heterogeneous partitions, i.e. partitions containing a different number of spatial locations.

odlomax · 2023-04-03T16:11:54Z

Closes #121

wdeconinck · 2023-04-06T13:14:59Z

Please @mo-lormi sign the CLA when you can.

src/atlas/functionspace/PointCloud.cc

MarekWlasak · 2023-04-11T08:02:45Z

If we ever get to use this for observation we will have cases where the longitude and latitude points are non unique. (We can have multiple observations at the same longitude/latitude) . It might be worth having a chat about this at the Atlas code sprint that is happening next week).

MarekWlasak · 2023-04-11T08:07:56Z

src/atlas/functionspace/PointCloud.cc

-                          array::make_view<idx_t, 1>(remote_index_).data(),
-                          REMOTE_IDX_BASE,
-                          ghost_.size());
+  ghost_(flds["ghost"]), remote_index_(flds["remote_index"]), partition_(flds["partition"]) {


I personally would have kept the old functionality for this constructor to allow for special cases.

@MarekWlasak please note that the constructor PointCloud(const FieldSet & flds) that uses four fields for the intialization has not been modified -- see lines 422-429.
The introduction of the function setupHaloExchange() and the check here allow to avoid redundant code by sharing the procedure to set up the halo exchange across constructors with different signature.

mo-lormi · 2023-04-21T10:52:55Z

Please @mo-lormi sign the CLA when you can.

... done

codecov-commenter · 2023-04-26T16:37:19Z

Codecov Report

Patch coverage: 99.51% and project coverage change: -0.55 ⚠️

Comparison is base (eaab18a) 77.54% compared to head (13a3227) 76.99%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #120      +/-   ##
===========================================
- Coverage    77.54%   76.99%   -0.55%     
===========================================
  Files          806      801       -5     
  Lines        57679    50544    -7135     
===========================================
- Hits         44725    38916    -5809     
+ Misses       12954    11628    -1326

Impacted Files	Coverage Δ
src/atlas/util/PolygonLocator.h	`68.25% <ø> (-3.37%)`	⬇️
src/tests/functionspace/test_pointcloud.cc	`100.00% <ø> (ø)`
src/atlas/functionspace/PointCloud.cc	`83.65% <98.63%> (+5.34%)`	⬆️
src/atlas/functionspace/PointCloud.h	`87.50% <100.00%> (-5.61%)`	⬇️
.../functionspace/test_pointcloud_haloexchange_2PE.cc	`100.00% <100.00%> (ø)`
.../functionspace/test_pointcloud_haloexchange_3PE.cc	`100.00% <100.00%> (ø)`

... and 567 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

wdeconinck

Hi @mo-lormi , Very good improvement! Just a few cosmetic changes requested.

wdeconinck · 2023-05-03T14:09:08Z

src/atlas/functionspace/PointCloud.h

-    PointCloud(const FieldSet&);  // assuming lonlat ghost ridx and partition present.
-
+    PointCloud(const Field&);
+    PointCloud(const Field&, const Field&);


It is good to keep the arguments named in above 2 lines to indicate what is expected without going into the ".cc" file.

wdeconinck · 2023-05-03T14:14:50Z

src/atlas/functionspace/PointCloud.h

 *
 * 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 immunities
- * granted to it by virtue of its status as an intergovernmental organisation
- * nor does it submit to any jurisdiction.


This comment cannot be removed

wdeconinck · 2023-05-03T14:15:08Z

src/tests/functionspace/test_pointcloud.cc

 *
 * 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 immunities
- * granted to it by virtue of its status as an intergovernmental organisation
- * nor does it submit to any jurisdiction.


this comment cannot be removed

wdeconinck · 2023-05-03T14:17:36Z

src/tests/functionspace/test_pointcloud_haloexchange_3PE.cc

+
+  const eckit::mpi::Comm& comm = atlas::mpi::comm();
+  std::size_t mpi_rank = comm.rank();
+  const std::size_t no_mpi_ranks = comm.size();


Cosmetic change: Please use "mpi_size" as name.

Actually, these lines were not used. I removed them.

wdeconinck · 2023-05-03T14:18:51Z

src/atlas/functionspace/PointCloud.cc

+void PointCloud::setupHaloExchange(){
+    const eckit::mpi::Comm& comm = atlas::mpi::comm();
+    std::size_t mpi_rank = comm.rank();
+    const std::size_t no_mpi_ranks = comm.size();


Cosmetic change: please use mpi_size as variable name.

wdeconinck

Thanks @mo-lormi all good now, ready to merge.

mo-lormi · 2023-05-03T14:54:19Z

Thanks @mo-lormi all good now, ready to merge.

Thanks ...

mo-lormi added 5 commits March 27, 2023 12:01

updated lon/lat layout

bfca20d

added new method to carry out the setup for the halo exchange

b770638

implemented new procedure to evaluate the fields 'partition' and 'rem…

c61f411

…ote_index' to be used for the halo exchange

added test associated with the halo exchange

6f56dcc

update 28.03.23-01

8e62758

github-actions bot added the contributor label Mar 28, 2023

mo-lormi added 4 commits March 30, 2023 08:08

added new method

4427c64

updated battery of tests; added new test;

91ab4ca

updated build procedure

2cf7e20

update 30.03.23-01

8a52e31

odlomax mentioned this pull request Apr 3, 2023

Generate remote-index and partition fields for PointCloud halo exchange. #121

Closed

mo-lormi added 2 commits April 4, 2023 09:58

removed redundant code

98ca50a

improved procedure to identify the locations

078f073

mo-lormi marked this pull request as ready for review April 6, 2023 09:03

wdeconinck self-assigned this Apr 6, 2023

MarekWlasak reviewed Apr 11, 2023

View reviewed changes

src/atlas/functionspace/PointCloud.cc Outdated Show resolved Hide resolved

MarekWlasak reviewed Apr 11, 2023

View reviewed changes

updated licence

da4e42b

update 21.04.23-01

13a3227

wdeconinck requested changes May 3, 2023

View reviewed changes

mo-lormi added 3 commits May 3, 2023 15:27

updated licence

962ce01

update 03.05.23-01

515d528

update 03.05.23-02

8920324

wdeconinck approved these changes May 3, 2023

View reviewed changes

wdeconinck added the approved-for-ci label May 3, 2023

wdeconinck merged commit 565c84d into ecmwf:develop May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade the halo exchange procedure for the function space 'PointCloud' #120

upgrade the halo exchange procedure for the function space 'PointCloud' #120

mo-lormi commented Mar 28, 2023 •

edited

Loading

FussyDuck commented Mar 28, 2023 •

edited

Loading

mo-lormi commented Mar 29, 2023 •

edited

Loading

odlomax commented Apr 3, 2023

wdeconinck commented Apr 6, 2023

MarekWlasak commented Apr 11, 2023

MarekWlasak Apr 11, 2023

mo-lormi Apr 17, 2023

mo-lormi commented Apr 21, 2023

codecov-commenter commented Apr 26, 2023

wdeconinck left a comment

wdeconinck May 3, 2023

mo-lormi May 3, 2023

wdeconinck May 3, 2023

mo-lormi May 3, 2023

wdeconinck May 3, 2023

mo-lormi May 3, 2023

wdeconinck May 3, 2023

mo-lormi May 3, 2023

wdeconinck May 3, 2023

mo-lormi May 3, 2023

wdeconinck left a comment

mo-lormi commented May 3, 2023

upgrade the halo exchange procedure for the function space 'PointCloud' #120

upgrade the halo exchange procedure for the function space 'PointCloud' #120

Conversation

mo-lormi commented Mar 28, 2023 • edited Loading

Description

Acceptance Criteria (Definition of Done)

FussyDuck commented Mar 28, 2023 • edited Loading

mo-lormi commented Mar 29, 2023 • edited Loading

odlomax commented Apr 3, 2023

wdeconinck commented Apr 6, 2023

MarekWlasak commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mo-lormi commented Apr 21, 2023

codecov-commenter commented Apr 26, 2023

Codecov Report

wdeconinck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdeconinck left a comment

Choose a reason for hiding this comment

mo-lormi commented May 3, 2023

mo-lormi commented Mar 28, 2023 •

edited

Loading

FussyDuck commented Mar 28, 2023 •

edited

Loading

mo-lormi commented Mar 29, 2023 •

edited

Loading