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.

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

Fix a few minor contour()-related issues #2951

Merged
merged 10 commits into from
Jul 7, 2022

Conversation

adeak
Copy link
Member

@adeak adeak commented Jul 7, 2022

This is a collection of minor things I came across while debugging #2901.

  1. The rng range in contour() should be a sorted (min, max) pair, otherwise we raise.
  2. The contour examples in the Flying Edges example should use a single contour value specified explicitly instead of specifying a degenerate range containing the same value twice (the original unsorted values probably only worked due to how VTK handles non-sorted cases).
  3. Explicit scalars passed to contour() should be allowed to be lists and other array-likes.
  4. The original error in case of missing point scalars was confusing, because it did this:
>>> pv.Sphere().contour(1, scalars='potato')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/adeak/pyvista/pyvista/core/filters/data_set.py", line 1588, in contour
    raise TypeError(
TypeError: Contour filter only works on Point data. Array (potato) is in the Cell data.

Since the same error appears when (i) the scalar name is invalid, and (ii) when it refers to cell scalars, and (iii) when the explicit scalars array passed is consistent with cell scalars, I decided to simplify it just to say that we need point scalars.

@adeak adeak added the maintenance Low-impact maintenance activity label Jul 7, 2022
@github-actions github-actions bot added documentation Anything related to the documentation/website bug Uh-oh! Something isn't working as expected. labels Jul 7, 2022
banesullivan
banesullivan previously approved these changes Jul 7, 2022
@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #2951 (d38b8fc) into main (47ddcd9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2951   +/-   ##
=======================================
  Coverage   94.30%   94.30%           
=======================================
  Files          76       76           
  Lines       16527    16535    +8     
=======================================
+ Hits        15586    15594    +8     
  Misses        941      941           

@akaszynski
Copy link
Member

This is good to go as-is. Coverage will be fixed in #2952.

Recommending immediate merge once all but codecov is passing.

@adeak
Copy link
Member Author

adeak commented Jul 7, 2022

This is good to go as-is. Coverage will be fixed in #2952.

Recommending immediate merge once all but codecov is passing.

I was about to recommend merging yours first because there's probably overlap with this one for the scalars type checking things :) Whichever direction works for you.

(Even codecov should be good now.)

@akaszynski
Copy link
Member

akaszynski commented Jul 7, 2022

I'll merge mine first once approved.

@akaszynski akaszynski added this to the 0.35.0 milestone Jul 7, 2022
@akaszynski
Copy link
Member

Merging this with the hope to get 0.35.0 out by the end of the day. This breaks the 24H rule, but as this is a small bug fix, I'm sure we'll be forgiven.

@akaszynski akaszynski merged commit dc3e83c into pyvista:main Jul 7, 2022
@adeak
Copy link
Member Author

adeak commented Jul 7, 2022

For what it's worth I consider "impending release" to be an exception to review pedantry, see #2245 (reply in thread).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Uh-oh! Something isn't working as expected. documentation Anything related to the documentation/website maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants