Enforce scalar output for contour filter #2952
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2952 +/- ##
==========================================
+ Coverage 94.28% 94.30% +0.02%
==========================================
Files 76 76
Lines 16520 16527 +7
==========================================
+ Hits 15576 15586 +10
+ Misses 944 941 -3 |
| if scalars_name not in output.point_data: | ||
| if 'Unnamed_0' in output.point_data: |
There was a problem hiding this comment.
And what do we expect to be the situation when scalars_name not in output.point_data but 'Unnamed_0' not in output.point_data?
There was a problem hiding this comment.
Likely that there are two unnamed arrays, in which case we don't handle this at all.
I figure that edge case to be unlikely enough unless there's another filter that outputs a null named array and we're not handling that.
There was a problem hiding this comment.
Just so I understand correctly, is the problem that we wouldn't know which scalars array to grab? I assume there's no written guarantee that .get_array(0) is always the newest one (that's what I saw in 1 out of 1 quick tests). And I guess we'd still have to figure out the corresponding name.
OK, so as long as this is the best we can do it's all good. Thanks.
There was a problem hiding this comment.
That might be the best we can do. What you could do is check beforehand if any null arrays exist and then handle that, but this is quite an edge case.
If we can guarantee that the last null array added is the one we want, then we can just loop through the array names and pick the last "Unnamed_" array.
While we're on edge cases, here's another: what happens if someone names their array "Unnamed_0"?
There was a problem hiding this comment.
That might be the best we can do. What you could do is check beforehand if any null arrays exist and then handle that, but this is quite an edge case.
Yeah, I don't think it's worth the complexity.
While we're on edge cases, here's another: what happens if someone names their array "Unnamed_0"?
Ugh... all of these auto-array names should have leading double underscores, like the __rgba* one(s)...
| # NOTE: only point data is allowed? well cells works but seems buggy? | ||
| if field != FieldAssociation.POINT: | ||
| raise TypeError( | ||
| f'Contour filter only works on Point data. Array ({scalars}) is in the Cell data.' | ||
| ) |
There was a problem hiding this comment.
And a remark: let's clean this up for v0.36 (i.e. preference input kwargs vs forced point scalars, we should refactor some of these checks too probably).
There was a problem hiding this comment.
Agreed. Are we already tracking this in an issue?
There was a problem hiding this comment.
I can only see several specific instances of the general problem, e.g.
|
Ah, auto-merge was on... |
* upstream/main: Enforce scalar output for contour filter (pyvista#2952)
Resolve #2901 by enforcing scalar output array name.
Bonus: Permits
numpy.ndarrayand sequences inscalars.Thanks @adeak for pointing out the issues and producing a minimnum working example that replicates the error.