patchkernel: find all the closest cells to a given point #446
Conversation
src/patchkernel/surface_skd_tree.cpp
Outdated
| * argument will be set to the maximum representable distance | ||
| */ | ||
| void SurfaceSkdTree::findPointClosestCandidates(const std::array<double, 3> &point, double maxDistance, | ||
| double *distanceEstimate, Candidates &candidates) const |
src/patchkernel/surface_skd_tree.hpp
Outdated
| std::vector<std::size_t> *candidateIds = nullptr; | ||
| std::vector<double> *candidateMinDistances = nullptr; | ||
| std::vector<std::size_t> privateCandidateIds; | ||
| std::vector<double> privateCandidateMinDistances; |
There was a problem hiding this comment.
These should be a unique_ptr of a vector to avoid the allocation. Also, rename it to candidateTdsStorage, candidateMinDistancesStorage.
There was a problem hiding this comment.
The structure already contains the name candidates, we can remove it from its memebers.
There was a problem hiding this comment.
Can we come up with a more descriptive name? Like ClosestCellCandidates? Or ClosestCandidates?
There was a problem hiding this comment.
Move the declaration before the members.
src/patchkernel/surface_skd_tree.cpp
Outdated
| * argument will be set to the maximum representable distance | ||
| */ | ||
| void SurfaceSkdTree::findPointClosestCandidates(const std::array<double, 3> &point, double maxDistance, | ||
| double *distanceEstimate, Candidates &candidates) const |
There was a problem hiding this comment.
Since the Candidates already contain information about the distance of the candidates, we can move the distance estimate into the structure. We can call it maximumDistance.
There was a problem hiding this comment.
Or maxDistance to be coherent with the other member of the distance.
src/patchkernel/surface_skd_tree.hpp
Outdated
| std::vector<double> privateCandidateMinDistances; | ||
| }; | ||
|
|
||
| void findPointClosestCandidates(const std::array<double, 3> &point, double maxDistance, double *distance, Candidates &candidates) const; |
There was a problem hiding this comment.
This should be private or protected.
There was a problem hiding this comment.
It's already private, sorry for the noise.
marcocisternino
force-pushed
the
skdtree.find.closest.cells
branch
from
04419cb to
2f72b83
Compare
|
Can you confirm that the changes didn't slow-down the function? |
marcocisternino
force-pushed
the
skdtree.find.closest.cells
branch
from
2f72b83 to
8f9dec6
Compare
integration levelset tests run sometimes faster and sometimes slower than the current master, therefore, I can statistically confirm no changes in performance. |
marcocisternino
force-pushed
the
skdtree.find.closest.cells
branch
from
8f9dec6 to
2d5ba61
Compare
This pull request adds methods to SurfaceSkdTree to find all the closest cells to a given point. The aim is to provide external applications with the the chance to select the the closest cell using its criteria. Existing methods select the closest cell using a specific criterium: the cell more "aligned" with the line that connect the specified point and its projection onto the cell will be chosen.