Default set active vectors for filters #2433

MatthewFlamm · 2022-04-05T19:19:07Z

Overview

This PR implements a common helper to set a default for active_vectors on a mesh. This currently implements option #2 from #2334 (comment). Reproduced here:

Raise if there are no active vectors ("explicit is better than implicit").

Activate vectors if they are unambiguous, and maybe warn while doing so. Otherwise raise ("in the face of ambiguity refuse the temptation to guess").

Always activate some vector-looking data and warn the user that this happened ("practicality beats purity").

Details

The logic used here is to set a default active_vectors only if there is one vector-like array, i.e. shape (n, 3). It is still possible that the vector-like array is not truly a vector in the vtk sense (RGB array for example), but in this case the user should be aware of what is happening since it is unambiguous what the intent is.

MatthewFlamm · 2022-04-05T19:22:06Z

This PR is set to draft with one implementation for streamlines_from_source. I will continue adding in other filters that use active_vectors and will mark as ready for review at that point. The core functionality of this PR is ready for comment however.

MatthewFlamm · 2022-04-05T19:25:19Z

cc @adeak since we have discussed this in the past, in case you are able/willing to review.

MatthewFlamm · 2022-04-05T19:26:07Z

pyvista/utilities/helpers.py

+        else:
+            mesh.set_active_vectors(possible_vectors_cell[0], preference='cell')
+    elif n_possible_vectors < 1:
+        raise ValueError("No vector-like data available.")


I'm not sure that ValueError is the right Error to be raised in both cases.

Thanks for the ping, I've got this on my radar but I won't be able to give a closer look today.

I agree that the errors seem a bit different. If we stick to ValueError then that can work I guess. But we could consider defining some semantically correct ValueError subclasses, e.g. MissingDataError and AmbiguousDataError or whatever. (There's a school of thought that exceptions shouldn't be distinguished by error messages, and instead one should make liberal use of exception subclasses.)

I like this suggestion, but I need to think about it. I also think that having a ton of error subclasses will lead to confusion too. The art is getting the right level of generality vs. specificity.

Based on my comment below that glyph filter uses the active vector by default. It would be very useful to have specific Errors for the different types of problems here. Then we can catch those in glyph and provide an informative warning message.

codecov · 2022-04-05T19:32:08Z

Codecov Report

Merging #2433 (63a1041) into main (0856b08) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2433      +/-   ##
==========================================
- Coverage   93.70%   93.66%   -0.05%     
==========================================
  Files          74       75       +1     
  Lines       15994    16030      +36     
==========================================
+ Hits        14987    15014      +27     
- Misses       1007     1016       +9

tests/test_helpers.py

MatthewFlamm · 2022-04-06T15:08:06Z

The glyph filter unfortunately currently has a default to orient by the active vectors. If none are present, it simply does not orient and continues silently. We can either allow an error to be raised as in the current PR state, or we can add in an optional value to the helper to change the error to a warning. I'm leaning towards the second option so as to avoid a breaking change.

Edit: Instead of adding an option to use warnings instead of raising an Error, I've chosen to raise more specific Errors and then catch those in glyph. This allows for more control over specific warnings for each use case. It is not so straightforward to 're-throw' warnings without side effects.

pyvista/errors.py

adeak · 2022-04-06T16:13:04Z

We can either allow an error to be raised as in the current PR state, or we can add in an optional value to the helper to change the error to a warning. I'm leaning towards the second option so as to avoid a breaking change.

Since glyphing doesn't always need vectors (see also the current behaviour), I would also want to keep the option where no vectors are present. One could easily have a point cloud they want to colour/scale by scalars only, for instance. But if I understood your edit correctly, this is what you ended up doing (still haven't looked at the implementation yet).

MatthewFlamm · 2022-04-06T17:16:54Z

Since glyphing doesn't always need vectors (see also the current behaviour), I would also want to keep the option where no vectors are present

The problem is that the default behavior is to require vectors (orient=True), which would throw an error when no vectors are present, if we don't catch the error in glyph. But since the behavior defaults to no orientation when no active vectors are set, there are probably users who are unaware of this and are not using an orientation vector with the default glyph() behavior. Edit: This means that the existing default behavior could be to orient with vectors (if active vectors are already set) or don't orient with vectors if no active vectors are already set (even if there are available active vector-like arrays).

Additionally, I think orienting glyphs is a very common use case (arrow vector plots being an example), so there are probably lots of users using the default behavior with orientation as well. It is a bit of a pickle.

The options I see are, with my perceived pros and cons. As you can see, I am a proponent of option 2.

Raise an error in glyph when no default vector can be set -> unexpected breaking change for some users
Catch errors in glyph and instead throw a warning -> keep usability and warn users when orient is turned off. This is currently implemented. It has the least side effects for users IMO.
Change default behavior to 'orient=False' and throw an error -> errors are only thrown when users specifically request orient=True, so it is clear that the users are intending to orient
Change default from orient=True to orient=None, and warn users that the default will change to False in v0.X.0. Raise an error for no active vectors only when we switch to orient=False default. This will result in extra maintenance for users who use the default glyph with orientation.

MatthewFlamm · 2022-04-06T17:32:08Z

I have implemented in all the filters I could find using vectors, at least ones with input parameters named vectors.

This is fully ready for review.

PS. this is my first big-ish PR since pre-commit was added, and it is obvious from the lack of whitespace commits in this PR how much it has saved me.

adeak · 2022-04-06T18:26:06Z

Additionally, I think orienting glyphs is a very common use case (arrow vector plots being an example), so there are probably lots of users using the default behavior with orientation as well. It is a bit of a pickle.

I don't see it as that much of a pickle. We could choose to ignore orient altogether when there are no vectors, assuming that the user didn't really mean to orient if there's nothing to orient by.

The options I see are, with my perceived pros and cons. As you can see, I am a proponent of option 2.

Raise an error in glyph when no default vector can be set -> unexpected breaking change for some users

Catch errors in glyph and instead throw a warning -> keep usability and warn users when orient is turned off. This is currently implemented. It has the least side effects for users IMO.

Change default behavior to 'orient=False' and throw an error -> errors are only thrown when users specifically request orient=True, so it is clear that the users are intending to orient

Change default from orient=True to orient=None, and warn users that the default will change to False in v0.X.0. Raise an error for no active vectors only when we switch to orient=False default. This will result in extra maintenance for users who use the default glyph with orientation.

As we both agree we shouldn't raise when there are no vectors. This almost always means someone not touching orient because they aren't even thinking about orienting anything. This rules out 1. and 3. Personally, I don't think we should change the default to False, because when you do have vectors, you usually want to orient too.

From the above options I'd also go with 2 like you did. But I would consider another, additional change (which might be too eager for its own good): switching to orient=None, with the default staying True, and only raising when orient=True was explicitly passed and it can't be reconciled. This would mean that we don't break user code which is orient=True by default, but raise for users who explicitly want to orient but can't.

adeak · 2022-04-06T18:27:53Z

pyvista/core/filters/data_set.py

+            except AmbiguousDataError as err:
+                warnings.warn(
+                    f"{err}\nIt is unclear which one to use. orient will be set to False."
+                )
+                orient = False


And in this case, i.e. orient=True and there are too many potential vectors to use, I might even consider raising. This has to be a user error, and we have to force the user to do something (either disable orient, or choose vectors to orient by).

MatthewFlamm · 2022-04-06T19:09:16Z

From the above options I'd also go with 2 like you did. But I would consider another, additional change (which might be too eager for its own good): switching to orient=None, with the default staying True, and only raising when orient=True was explicitly passed and it can't be reconciled.

I'm leaning towards agreeing with your parenthetical, but I don't lean too far in that direction. Users might be puzzled why using glypyh(orient=True) differs from glyph() unless we spend a lot of lines in the docstring explaining this. It seems more straightforward to me to have a single behavior.

I will wait for more input on this before making more changes.

pyvista/utilities/helpers.py

adeak · 2022-04-07T20:31:22Z

pyvista/utilities/helpers.py

+            f"Multiple vector-like data available: {possible_vectors}.\n"
+            "Set one as active using DataSet.set_active_vectors(name)"


I wonder how confusing this would be if there's a single point vectors and a single cell vectors with the same name (might actually be more common than expected). We'd print the same name twice, and the set_active_vectors call would probably also need a preference kwarg to distinguish the two.

But I have no idea how to make this more descriptive for this case while keeping it concise.

We don't need to set a preference to call this function because we have, so far, taken the decision to raise an error if there is more than one vector-like array.

It would be most complete to print the vector-like arrays separately for points and cells. But we don't do this for usage like mesh.array_names so I don't know.

With the preference I just meant the recommendation in the error message: "Set one as active using DataSet.set_active_vectors(name)". But again it would be cumbersome to try and add the preference distinction to the message (to the effect of "also don't forget to choose the right preference if you have a name clash").

We could print cell and point vectors separately and then use DataSet.set_active_vectors(name, preference=type) something like this. This gives the user two pointers without having to be super explicit.

adeak · 2022-04-07T20:32:54Z

tests/test_filters.py

@@ -631,7 +631,15 @@ def test_glyph(datasets, sphere):
        geom=geoms, scale='arr', orient='Normals', factor=0.1, tolerance=0.1, progress_bar=True
    )
    assert sphere.glyph(geom=geoms[:1], indices=[None], progress_bar=True)
-    assert sphere_sans_arrays.glyph(geom=geoms, progress_bar=True)
+
+    with pytest.warns(Warning):  # tries to orient but no orientation vector available


I don't want to go overboard, but do we want to define a custom DataWarning/ActiveDataWarning or something to be emitted in these cases? 😄 (I'm still not great at naming things.)

I'm thinking that the warning should indicate that the input parameter is changed/ignored. This would be a general and fairly useful warning type. I'm blanking on a concise name too.

Good idea making it broader. Let's think about an appropriate name...

I am torn in two directions.

One is something like ActiveAttributesWarning, since we cannot set an active attribute in the vtk sense. If this too vtkish, I like your ActiveDataWarning. These are the cause of the warning.

To go another way, something like ParameterChangeWarning to indicate the effect of what is happening.

I think we can resolve this later, but as-is, this is sufficient.

pyvista/utilities/helpers.py

tests/test_helpers.py

Co-authored-by: Andras Deak <adeak@users.noreply.github.com>

…yvista into default_active_vectors

MatthewFlamm · 2022-04-08T01:59:46Z

I'm starting to think that we may want to leave glyph out of this PR, or widen the scope of this PR to include scalars. glyph has the same problem with scale as it does with orient. It will be weird to have warnings for one but not the other

akaszynski · 2022-04-11T11:52:06Z

@MatthewFlamm, is this ready for review?

MatthewFlamm · 2022-04-11T12:44:51Z

@MatthewFlamm, is this ready for review?

Yes

akaszynski

Great work, and thanks for the review @adeak

adeak

Potential future work in

But I agree with @akaszynski that this is good as-is. Thanks!

akaszynski · 2022-04-13T19:56:32Z

But I agree with @akaszynski that this is good as-is. Thanks!

Trying to avoid PRs becoming too long lived (big believer in trunk based development and short lived branches).

Agree with follow-ups.

MatthewFlamm added 2 commits April 5, 2022 15:09

add set_default_active_vectors and test

889d68a

use in streamlines_from_source

64e9bf3

MatthewFlamm commented Apr 5, 2022

View reviewed changes

MatthewFlamm added 2 commits April 5, 2022 20:00

fix dimension check for possible vectors

7544f62

typing and docstring

7246fb4

tkoyama010 reviewed Apr 6, 2022

View reviewed changes

tests/test_helpers.py Show resolved Hide resolved

MatthewFlamm added 2 commits April 6, 2022 07:34

test multiple with cell and point

424a4b0

use in glyph

7022f5b

Use more specific errors and catch in glyph with informative warnings.

7c748f4

MatthewFlamm commented Apr 6, 2022

View reviewed changes

pyvista/errors.py Show resolved Hide resolved

MatthewFlamm added 2 commits April 6, 2022 13:24

use in warp_by_vector

d087652

use in streamlines_evenly_spaced_2D

c74c3a6

MatthewFlamm marked this pull request as ready for review April 6, 2022 17:31

set orient to False when promised

7e6aebf

adeak reviewed Apr 6, 2022

View reviewed changes

tkoyama010 added the maintenance Low-impact maintenance activity label Apr 7, 2022

adeak reviewed Apr 7, 2022

View reviewed changes

MatthewFlamm and others added 4 commits April 7, 2022 20:26

use specific errors in test and add same name test

bca5c23

Update pyvista/utilities/helpers.py

6e37f8d

Co-authored-by: Andras Deak <adeak@users.noreply.github.com>

Apply suggestions from code review

2f2b6c8

Co-authored-by: Andras Deak <adeak@users.noreply.github.com>

Merge branch 'default_active_vectors' of https://github.com/pyvista/p…

76527a3

…yvista into default_active_vectors

Update docstring from review

11501db

MatthewFlamm closed this Apr 8, 2022

MatthewFlamm reopened this Apr 8, 2022

distinguish between cell and point in error

63a1041

akaszynski mentioned this pull request Apr 12, 2022

depreciate the existing errors in pyvista.core.errors #2454

Open

akaszynski approved these changes Apr 13, 2022

View reviewed changes

adeak approved these changes Apr 13, 2022

View reviewed changes

akaszynski merged commit b50614a into main Apr 13, 2022

akaszynski deleted the default_active_vectors branch April 13, 2022 19:56

akaszynski mentioned this pull request Apr 14, 2022

documentation failing due to missing active vectors #2467

Closed

MatthewFlamm mentioned this pull request Apr 17, 2022

Set default scalars for filters #2474

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default set active vectors for filters #2433

Default set active vectors for filters #2433

MatthewFlamm commented Apr 5, 2022 •

edited

MatthewFlamm commented Apr 5, 2022

MatthewFlamm commented Apr 5, 2022

MatthewFlamm Apr 5, 2022

adeak Apr 5, 2022

MatthewFlamm Apr 5, 2022

MatthewFlamm Apr 6, 2022

codecov bot commented Apr 5, 2022 •

edited

MatthewFlamm commented Apr 6, 2022 •

edited

adeak commented Apr 6, 2022

MatthewFlamm commented Apr 6, 2022 •

edited

MatthewFlamm commented Apr 6, 2022 •

edited

adeak commented Apr 6, 2022 •

edited

adeak Apr 6, 2022

MatthewFlamm commented Apr 6, 2022

adeak Apr 7, 2022

MatthewFlamm Apr 8, 2022

adeak Apr 8, 2022

MatthewFlamm Apr 8, 2022

adeak Apr 7, 2022

MatthewFlamm Apr 8, 2022

adeak Apr 8, 2022

MatthewFlamm Apr 8, 2022

akaszynski Apr 13, 2022

MatthewFlamm commented Apr 8, 2022 •

edited

akaszynski commented Apr 11, 2022

MatthewFlamm commented Apr 11, 2022

akaszynski left a comment

adeak left a comment

akaszynski commented Apr 13, 2022

		f"Multiple vector-like data available: {possible_vectors}.\n"
		"Set one as active using DataSet.set_active_vectors(name)"

Default set active vectors for filters #2433

Default set active vectors for filters #2433

Conversation

MatthewFlamm commented Apr 5, 2022 • edited

Overview

Details

MatthewFlamm commented Apr 5, 2022

MatthewFlamm commented Apr 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 5, 2022 • edited

Codecov Report

MatthewFlamm commented Apr 6, 2022 • edited

adeak commented Apr 6, 2022

MatthewFlamm commented Apr 6, 2022 • edited

MatthewFlamm commented Apr 6, 2022 • edited

adeak commented Apr 6, 2022 • edited

Choose a reason for hiding this comment

MatthewFlamm commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewFlamm commented Apr 8, 2022 • edited

akaszynski commented Apr 11, 2022

MatthewFlamm commented Apr 11, 2022

akaszynski left a comment

Choose a reason for hiding this comment

adeak left a comment

Choose a reason for hiding this comment

akaszynski commented Apr 13, 2022

MatthewFlamm commented Apr 5, 2022 •

edited

codecov bot commented Apr 5, 2022 •

edited

MatthewFlamm commented Apr 6, 2022 •

edited

MatthewFlamm commented Apr 6, 2022 •

edited

MatthewFlamm commented Apr 6, 2022 •

edited

adeak commented Apr 6, 2022 •

edited

MatthewFlamm commented Apr 8, 2022 •

edited