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

Set default scalars for filters #2474

Merged
merged 18 commits into from
May 6, 2022
Merged

Set default scalars for filters #2474

merged 18 commits into from
May 6, 2022

Conversation

MatthewFlamm
Copy link
Contributor

@MatthewFlamm MatthewFlamm commented Apr 17, 2022

Overview

Followup to #2433, this time for scalars.

Details

Adding data to DataSets automatically makes them active, but it is still possible that there are no active scalars, e.g. if there is no data at all

@MatthewFlamm MatthewFlamm marked this pull request as draft April 17, 2022 20:33
@MatthewFlamm
Copy link
Contributor Author

I will keep this as draft until it is used in all the filters I can find.

@codecov
Copy link

codecov bot commented Apr 17, 2022

Codecov Report

Merging #2474 (d4b7025) into main (87ea7d5) will decrease coverage by 0.02%.
The diff coverage is 92.68%.

@@            Coverage Diff             @@
##             main    #2474      +/-   ##
==========================================
- Coverage   93.72%   93.69%   -0.03%     
==========================================
  Files          75       75              
  Lines       16120    16159      +39     
==========================================
+ Hits        15108    15140      +32     
- Misses       1012     1019       +7     

@tkoyama010 tkoyama010 added the maintenance Low-impact maintenance activity label Apr 18, 2022
@adeak
Copy link
Member

adeak commented Apr 18, 2022

Since you're already working in this ballpark, I got this warning locally on main:

tests/test_demos.py::test_plot_glyphs
tests/plotting/test_plotting.py::test_plot_compare_four
  /home/adeak/pyvista/pyvista/core/filters/data_set.py:1999: UserWarning: No vector-like data to use for orient. orient will be set to False.
    warnings.warn("No vector-like data to use for orient. orient will be set to False.")

Probably just a matter of a manual False flag, but I haven't been able to investigate yet.

@akaszynski
Copy link
Member

tests/test_demos.py::test_plot_glyphs
tests/plotting/test_plotting.py::test_plot_compare_four
/home/adeak/pyvista/pyvista/core/filters/data_set.py:1999: UserWarning: No vector-like data to use for orient. orient will be set to False.
warnings.warn("No vector-like data to use for orient. orient will be set to False.")

Should be fixed in #2460. I'm tempted to merge that so we avoid duplication.

@MatthewFlamm
Copy link
Contributor Author

It is weird to fix in #2460, I agree we don't need two semi related PRs that fix this.

@akaszynski
Copy link
Member

It is weird to fix in #2460, I agree we don't need two semi related PRs that fix this.

Agreed. It was bothering me, but I should have split it up.

@adeak
Copy link
Member

adeak commented Apr 18, 2022

OK, so I think what @akaszynski fixed in #2460 was a doc build warning coming from https://github.com/pyvista/pyvista/pull/2460/files#diff-355952f3009b3d15d0d85ebb510556c3db59722b98c0d5c7d57abdb2b6ea5363L454 .

What I said was a test run, somewhere in tests/. So there's still something to fix here.

Ah, no, I missed the changed tests too in https://github.com/pyvista/pyvista/pull/2460/files#diff-14cb3a563d7f5dd0da9f56abbb5a24503d19670735bf67d24036f9524529caaeL1520

Might be all good, sorry for the noise.

@MatthewFlamm MatthewFlamm marked this pull request as ready for review April 19, 2022 14:32
@MatthewFlamm
Copy link
Contributor Author

This is ready for review.

When adding all the filters I could find, I noticed that some filters use SetInputArrayToProcess and some filters leave it to the vtk algorithm to use the active scalars by default. It would be good to eventually align on a preferred usage. But, this is out of scope for this PR.

Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

I added one final test, but otherwise this is good to go.

@akaszynski akaszynski enabled auto-merge (squash) May 6, 2022 23:03
@akaszynski akaszynski merged commit c49a6ab into main May 6, 2022
@akaszynski akaszynski deleted the maint/set-default-scalars branch May 6, 2022 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants