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.

Sign up for GitHub

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

Jump to bottom

Add utility function returning point indices which are not found #15319

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add utility function returning point indices which are not found #15319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/deal.II/base/mpi_remote_point_evaluation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,12 @@ namespace Utilities
bool
point_found(const unsigned int i) const;

/**
* Return a list of point indices which have not been found.
*/
std::vector<unsigned int>
Copy link
Member

Choose a reason for hiding this comment

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

I must admit I am not particular fond of this new function, since I don't know how it simplifies user code (as you also indicated in your comment #15248 (comment)).

The use case as far as I read is (with the new function):

std::vector<Point> not_found_points;

const auto not_found_points_indices = rpe.get_point_indices_not_found();

for(const auto i : not_found_points_indices)
  not_found_points.push_back(points[i]);

and with old functionalities

std::vector<Point> not_found_points;

for(unsigned int i = 0; i < points.size(); ++i)
  if(rpe.point_found(i))
    not_found_points.push_back(points[i]);

The new implementation does not really reduce the number of lines in user code, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's OK for me to drop this. Since I have been working with RPE quite much, I just thought I give #15248 a try. Let's wait for @nfehn to confirm this is not as helpful as intended, then we can close this PR and the linked issue, since there is no way to hand back a std::vector<Point<dim>> because this information is simply not known in RPE.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the second code variant termed "old functionalities" has forgotten a not / !? Avoiding negations in user code is always good in my opinion.

I definitely see value in functions like

auto const point_indices_not_found = rpe.get_point_indices_not_found();
auto const points_not_found = rpe.get_points_not_found(points);

Why should one write 3 lines of code in potentially many user codes at many places. This is a potential source of error, as the above code example #15319 (comment) shows.

From a design perspective, it is problematic in both code examples that the index handling is done in user code. What if the points vector is manipulated in between? In my opinion, the single-responsibility design principle would call for having all parameters involved in a single function call to RPE. The current design also causes additional documentation effort, see https://dealii.org/current/doxygen/deal.II/classUtilities_1_1MPI_1_1RemotePointEvaluation.html#ad2e596c946ea413c48e04fa9143b01d2.

For these reasons, it would be natural to either store the points in RPE, or to let the setup function return points_not_found information (to be used by the user or not). I am thinking of

auto const success_or_failure_data = rpe.reinit(...);

Success/failure data could be a bool all_points_found, a list of indices or points, etc., to be discussed ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should internally store data which is not needed in the class, but we could introduce reinit_and_get_not_found_points() in addition to reinit().
@peterrum Any objections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any other opinions on this?

get_point_indices_not_found() const;

/**
* Return the Triangulation object used during reinit().
*/
Expand Down
17 changes: 17 additions & 0 deletions source/base/mpi_remote_point_evaluation.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,23 @@ namespace Utilities



template <int dim, int spacedim>
std::vector<unsigned int>
RemotePointEvaluation<dim, spacedim>::get_point_indices_not_found() const
{
if (all_points_found_flag)
return {};

std::vector<unsigned int> points_not_found;
for (unsigned int i = 0; i < point_ptrs.size() - 1; ++i)
if (!point_found(i))
points_not_found.push_back(i);

return points_not_found;
}



template <int dim, int spacedim>
const Triangulation<dim, spacedim> &
RemotePointEvaluation<dim, spacedim>::get_triangulation() const
Expand Down