Skip to content

Conversation

@loumalouomega
Copy link
Member

@loumalouomega loumalouomega commented Oct 25, 2023

📝 Description

#11416 + addressed comments

This PR introduces a new class named SpatialSearchResultContainer, SpatialSearchResultContainerVector as well as SearchWrapper. The two first classes provides storage and operations related to spatial search results. In addition SearchWrapper class implements an interface for serial/MPI search for different search objects, in current PR adding GeometricalObjectBins and the available trees.

The SpatialSearchResultContainer class stores:

  1. Local pointers of results (in a PointerVectorSet)
  2. Global pointers (empty if not synchronized)
  3. GlobalPointerCommunicator pointer (to synchronize the global pointers)

The class includes functions such as, SynchronizeAll, HasResult, GetResultIndices, AddResult... The interesting part is that integrates a global pointer communicator, so it is possible to calculate anything passing a functor. The most common operations required are already implemented: GetResultShapeFunctions, GetResultIndices and GetResultCoordinates,

The SynchronizeAll method synchronizes the container between partitions. The AddResult method adds a result to the local pointers. There is a push_back which does the same.

The SpatialSearchResultContainerVector class stores a std::vector of SpatialSearchResultContainer pointers. In the SearchWrapper class the index of the vector corresponds with the node id when using nodes and just a counter when simply searching.

Considering general data communicator instead of sub data communicator

TODO: Using coloring utilities instead of manual coloring

🆕 Changelog

@loumalouomega loumalouomega added Enhancement Kratos Core Testing Parallel-MPI Distributed memory parallelism for HPC / clusters Feature labels Oct 25, 2023
@loumalouomega loumalouomega requested review from a team as code owners October 25, 2023 12:17

#ifndef KRATOS_BASE_DATA_COMMUNICATOR_DECLARE_ALLREDUCE_LOC_INTERFACE_FOR_TYPE
#define KRATOS_BASE_DATA_COMMUNICATOR_DECLARE_ALLREDUCE_LOC_INTERFACE_FOR_TYPE(...) \
virtual std::pair<__VA_ARGS__, int> MinLocAll(const __VA_ARGS__& rLocalValue) const { return std::pair<__VA_ARGS__, int>(rLocalValue, 0); } \
Copy link
Member

Choose a reason for hiding this comment

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

just to be sure:

the data_communicator changes being reviewed in a different PR, am i correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

const auto& r_local_min = r_local_bb.GetMinPoint();

// Getting max values
r_max[0] = mrDataCommunicator.MaxAll(r_local_max[0]);
Copy link
Member

Choose a reason for hiding this comment

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

unimportant question (just ignore if you feel) but, isn't there a vector version of this? i mean one that considers the vector of components instead of each component separately

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, if I can simplify the code that would be welcomed

if (mrDataCommunicator.IsDistributed()) {
DistributedSearchInRadius(rPoint, Radius, rResults, SyncronizeResults);
} else { // Call serial version
SerialSearchInRadius(rPoint, Radius, rResults, SyncronizeResults);
Copy link
Member

Choose a reason for hiding this comment

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

why do you pass the "SyncronizeResults" flag to the serial version? in that case it can only be in sync

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me check if I can simplify code, originally this was supposed to be a derived class and when refactored the code maybe I copy pasted to much

Copy link
Member

@RiccardoRossi RiccardoRossi left a comment

Choose a reason for hiding this comment

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

I have the impression that this is really not ready for prime time.

@loumalouomega can we have a call to speak about this ?

@loumalouomega
Copy link
Member Author

After all PR are merged, this is ready for final review

@loumalouomega
Copy link
Member Author

@pooyan-dadvand @RiccardoRossi I removed the shared_ptr

@RiccardoRossi RiccardoRossi dismissed their stale review November 13, 2023 14:46

leaving the review to @pooyan

@loumalouomega
Copy link
Member Author

@RiccardoRossi you mentioned a random pooyan instead of @pooyan-dadvand

Copy link
Member

@roigcarlo roigcarlo left a comment

Choose a reason for hiding this comment

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

Aside for the fact that I still find the search_wrapper odd, I see no other problem.

@RiccardoRossi for me this is ok to merge...

@loumalouomega
Copy link
Member Author

I think this is definetively ready @pooyan-dadvand


// Remove the non lowest rank results
Timer::Start("ParallelSpatialSearch::KeepOnlyLowestRankResult");
KeepOnlyLowestRankResult(rResults);
Copy link
Member Author

Choose a reason for hiding this comment

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

@pooyan-dadvand I reverted this, because I didn't remember, but search are locals, for performance reasons, and it is not possible to use a syncronous data communicator inside the search, otherwise it gets stuck, and thats why the last CI after 9726a34 was failing.

This is far more efficient than performing a global search with all partitions at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Feature Kratos Core Parallel-MPI Distributed memory parallelism for HPC / clusters Testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants