Do not set scalars as active when adding

[banesullivan]

Overview

Helps address #1897 and should solve #3486

Details

These changes prevent a scenario where a mesh may already have active scalars and adding a new array automatically gets set as active. #1897 and #3486 outline several places where this behavior has unintended consequences

The behavior is as follows when adding a new scalar array (point or cell data):

• If no array is currently active, the added array becomes active
• If an array is currently active, the added array is not set as active

Example

import pyvista as pv

# Issue arrises with CELL data
mesh = pv.Wavelet().ptc()

p = pv.Plotter(notebook=0)
p.add_mesh_clip_plane(mesh, crinkle=True, normal=(-1, 1, -1))
p.show()

main	this branch

[banesullivan]

These changes conflict with the test_active_scalars_remain in tests/test_plotter.py -- seeking feedback from @pyvista/developers

[MatthewFlamm]

I think there should be a comprehensive proposal here before moving forward. Otherwise we might want to change course again with another breaking change. This should be documented somewhere. I can think of two possibilities here for discussion, here I just focus on scalars:

1. Never automatically set active scalars when assigning data, try to adhere to this philosophy as much as possible in internal code. Document when not possible.
2. Automatically set active scalars when none currently exists. <- current PR.
   a. When filters or internal code assigns data, and no data currently exists, set as active. This is consistent, but I think will be surprising to users with a side effect. This happens throughout the entire code base.
   b. When filters or internal code assigns data, and no data currently exists, do not set as active. Document when not possible. We could consider a decorator to store active scalars then reset active scalars after a filter or method operation to ensure this.

I like either option 1 or option 2b.

The following applies if option 2 or other implementation that sets active scalars. What about other array types? For example, what about when a (N, 3) array is saved? Should this be an active scalar when no other scalar exists or should it be active vector similarly, or both? What about Normals, Tensors, etc? We don't support all of these the same today, but it is worth thinking about to avoid further breaking changes like this. I think scalars, vectors, and Normals are the most common usages. I would vote for implementing this for the generic data, scalars, vectors, and tensors. Normals are a special data type. Do we support Tensors fully today?

I propose the logic: if there is no active scalar/vector/tensor set and data is saved that is of suitable shape, set it as active. This would result in:

• If there is no data, and a (N,3) array is saved, it will be set as both active scalar and vector.
• If there is an active scalar, and a (N,3) is saved, it will be set as only the active vector.

[akaszynski]

I like either option 1...

Never automatically set active scalars when assigning data, try to adhere to this philosophy as much as possible in internal code. Document when not possible.

I genuinely prefer this as this follows "Explicit is better than implicit" where we minimize side effects as much as possible. There's of course a lack of convenience here, which is why we proposed making them active (and keeping them active) when using set_array in something like:

cube = pv.Cube()
cube['scalars'] = cube.points[:, 0]

This makes it really convenient when plotting with cube.plot(), and I'm happy with this behavior, that users can easily override with plot(color=<colorlike>).

This leaves us with either 2a or 2b

... option 2b.
When filters or internal code assigns data, and no data currently exists, do not set as active. Document when not possible. We could consider a decorator to store active scalars then reset active scalars after a filter or method operation to ensure this.

This seems like a compromise between "assign always" and "assign never". When a user sets data using __set_item__ it's fairly clear that they're adding data and already expecting some sort of side effect of at least adding another array to the point/cell data. However, this assumption (to me) starts to break down when using filters when it's not clear which arrays are being added and why some are being made active. However, using a filter is still expected to operate on the data (somehow) and filters like compute_normals can be reasonably expected to set scalars to active.

That just leads us with 2a:

if there is no active scalar/vector/tensor set and data is saved that is of suitable shape, set it as active.

I prefer this behavior, the behavior proposed to keep (and really patch) in this PR. However, we need to consider @MatthewFlamm's comments regarding vector data:

If there is no data, and a (N,3) array is saved, it will be set as both active scalar and vector.
If there is an active scalar, and a (N,3) is saved, it will be set as only the active vector.

Fully agree there. Right now we only address scalars. I recommend adding this in a follow-up PR and keeping this PR for the release since it's a bug fix of (I hope) agreed upon behavior.

[codecov]

Codecov Report

Merging #3535 (86acfe3) into main (ecd1dc6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #3535   +/-   ##
=======================================
  Coverage   95.17%   95.17%           
=======================================
  Files          83       83           
  Lines       18571    18577    +6     
=======================================
+ Hits        17675    17681    +6     
  Misses        896      896           

[larsoner]

Agreed with @akaszynski's summary + plan 👍

[akaszynski]

Given the general consensus, I'm recommending approval for this. Summarizing:

• Agree with @banesullivan's approach of setting active scalars based on the "first" added if there are no active ones. It's a trade-off between usability and having an explicit API.
• We need a follow-up PR that does the same thing for vectors.

Ignoring the MNE CI failure. I'm sure @larsoner is aware and we passed before the jupyter warnings started causing the issue.

[akaszynski]

@MatthewFlamm, I know you're busy, but please let me know if you're fine with this and we can move forward with the follow-up PR.

[MatthewFlamm]

I'm okay with this PR. Im not convinced that we should greedily activate generated data as much as possible however and this can wait for a future conversation. For example, it doesn't make sense to me to make the data resulting from compute_normals active scalars or vectors.

I suppose I'm proposing that we should set active X when the data being generated is the primary expected outcome of an operation. Here X is whatever type is relevant, e.g. scalars or normals or... If the data generated is ancillary, then do not make it active. For example, in some cases the original ID of a point/cell may be stored during a geometrical operation. Does it makes sense to make these active scalars?

[banesullivan]

I think we're all on the same page. This PR can be the first step to improving active scalars (and vectors/tensors to follow).

IMO, I think this PR is ready to merge as is given @akaszynski's plan above

[akaszynski]

Let's merge and open an issue with "the plan".