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

Allow default-initialized distributed tree #1040

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Mar 1, 2024

This would, for example, allow storing DistributedTree in user classes for later initialization.

@aprokop aprokop added the enhancement New feature or request label Mar 1, 2024
test/tstDistributedTree.cpp Outdated Show resolved Hide resolved
test/tstDistributedTree.cpp Outdated Show resolved Hide resolved
@aprokop
Copy link
Contributor Author

aprokop commented Mar 2, 2024

All tests pass except for SYCL. Trying to address the latter in #1041.

Copy link
Contributor

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

The empty tree state still does not feel right to me.
We did not spell out any precondition on the getComm() member. Should we be initializing to MPI_COMM_NULL or such?

@aprokop
Copy link
Contributor Author

aprokop commented Mar 12, 2024

We did not spell out any precondition on the getComm() member. Should we be initializing to MPI_COMM_NULL or such?

Addressed. The preconditions to calling MPI_Comm_rank are still checked because MPI standard says that most calls cannot be called with MPI_COMM_NULL.

Whether getComm() should be a public member is an open question, and outside of the scope of this PR.

@dalg24
Copy link
Contributor

dalg24 commented Mar 12, 2024

Please do this instead

diff --git a/src/ArborX_DistributedTree.hpp b/src/ArborX_DistributedTree.hpp
index f31ea1b8..229b60cf 100644
--- a/src/ArborX_DistributedTree.hpp
+++ b/src/ArborX_DistributedTree.hpp
@@ -50,13 +50,7 @@ public:
   using bounding_volume_type = BoundingVolume;
   using value_type = typename BottomTree::value_type;

-  DistributedTreeBase()
-  {
-    _comm_ptr.reset([]() {
-      auto p = std::make_unique<MPI_Comm>(MPI_COMM_NULL);
-      return p.release();
-    }());
-  }
+  DistributedTreeBase() = default;

   template <typename ExecutionSpace, typename... Args>
   DistributedTreeBase(MPI_Comm comm, ExecutionSpace const &space,
@@ -115,7 +109,8 @@ protected:
 private:
   friend struct Details::DistributedTreeImpl;

-  std::shared_ptr<MPI_Comm> _comm_ptr;
+  std::shared_ptr<MPI_Comm> _comm_ptr{
+      std::make_unique<MPI_Comm>(MPI_COMM_NULL)};
   BottomTree _bottom_tree; // local
   TopTree _top_tree;       // replicated
   size_type _top_tree_size{0};

Co-authored-by: Damien L-G <dalg24@gmail.com>
@aprokop aprokop merged commit 33c8205 into arborx:master Mar 13, 2024
1 of 2 checks passed
@aprokop aprokop deleted the default_distributed_tree branch March 13, 2024 01:51
@aprokop aprokop mentioned this pull request Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants