Add method to check if point is inside cell

[MatthewFlamm]

Overview

Add method to check if point is inside a specific cell. This was motivated partly by the discussion in #2703 (reply in thread)

Details

Related to #2897.

Should we allow a Sequence of points (if more than one is provided we can return a numpy ndarray)? If so, this might complicate any plans to allow for Iterables, Sequences, or similar of cell indices.

[tkoyama010]

Thanks for adding a useful method.

[codecov]

Codecov Report

Merging #2905 (8eb80f5) into main (7092937) will decrease coverage by 0.04%.
The diff coverage is 96.96%.

@@            Coverage Diff             @@
##             main    #2905      +/-   ##
==========================================
- Coverage   94.12%   94.08%   -0.05%     
==========================================
  Files          76       76              
  Lines       16525    16558      +33     
==========================================
+ Hits        15554    15578      +24     
- Misses        971      980       +9     

[MatthewFlamm]

Unlike DataSet.find_containing_cell, there is only minor efficiency gained by allowing multiple points to be supplied here. The inefficiency here is multiple calls to GetCell(ind) and GetPoints().GetNumberOfPoints() (I'm assuming the calls to vtk.mutable are cheap), which would be avoided if we allowed multiple points to be supplied to this function. Should we implement multiple points here? I'm not sure based on some discussions about also supporting multiple cells here #2897 (comment). If we support multiple points and multiple cells, then it becomes unclear, at least to me, what the 2D output structure should look like.

cc @adeak and @banesullivan

[tkoyama010]

We need to wait for a reply to the above, otherwise, it is LGTM.

[adeak]

Unlike DataSet.find_containing_cell, there is only minor efficiency gained by allowing multiple points to be supplied here. The inefficiency here is multiple calls to GetCell(ind) and GetPoints().GetNumberOfPoints() (I'm assuming the calls to vtk.mutable are cheap), which would be avoided if we allowed multiple points to be supplied to this function. Should we implement multiple points here? I'm not sure based on some discussions about also supporting multiple cells here #2897 (comment). If we support multiple points and multiple cells, then it becomes unclear, at least to me, what the 2D output structure should look like.

My main reason for contemplating multiple points/cells as input is not that of performance (I don't typically associate VTK's API with vectorization), but rather convenience. If one has more than one point to check it would be nicer to call the method once with every point, rather than using something like a list comprehension outside the function, point by point.

This ties in with your question: if the API becomes confusing in the "multiple points, multiple cells" case then we should just forbid it. It's not convenient to have to look up the docs to see how it behaves.

That being said, I could imagine that if either points or cells are multiple then we return an array of bools (as large as the number of input points or cells), and if both are sequences then we return a 2d array of shape (n_points, n_indices) or (n_indices, n_points). But again, this might be too smart for its own good.

[MatthewFlamm]

I agree on the convenience factor when using one or the other. I find the convenience factor will drop when using both together. I will never remember which comes first in the indices. But, maybe this is a rare enough use case that it isn't worth trying to over optimize the API. I'll implement multiple points

[akaszynski]

Since 0.35 will be released soon, I'd like this to wait for the next minor (0.36) release. Best to not add any last second features.

Good news is we won't wait 3 months to get out the next release. There are a few critical features that we need much earlier, so expect 0.36.0 to be out in a month.

[adeak]

Oh, I forgot that this was waiting for me, I'm sorry @MatthewFlamm.

@akaszynski I think this is good to go, if you can give me ten minutes to make sure.

[akaszynski]

Oh, I forgot that this was waiting for me, I'm sorry @MatthewFlamm.

@akaszynski I think this is good to go, if you can give me ten minutes to make sure.

Absolutely. Approve and I'll merge when you're ready.

[adeak]

Two cosmetic remarks here that we could do:

1. skip coverage for the esoteric error case to make coverage happy
2. also add the point index i in the error.

I find it much more valuable to get this feature out in v0.35 than to fix this "should almost never happen" edge case and add a doc-build-length delay. We can follow up for the subsequent release if we want to.

Approving, thanks and sorry for forgetting about this.