Add utility function returning point indices which are not found #15319
Conversation
| /** | ||
| * Return a list of point indices which have not been found. | ||
| */ | ||
| std::vector<unsigned int> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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?
Might close #15248