8000 Deprecate selector `visible` attribute by ericpre · Pull Request #22167 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Deprecate selector visible attribute #22167

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 23, 2022
Merged

Deprecate selector visible attribute #22167

merged 2 commits into from
Jun 23, 2022

Conversation

ericpre
Copy link
Member
@ericpre ericpre commented Jan 9, 2022 •
edited
Loading

PR Summary

As noticed in #22146, the use of the selector visible attribute is ambiguous. This PR deprecate the visible attribute in favour of using set_visible/get_visible to be consistent with other matplotlib objects.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • [n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@ericpre ericpre force-pushed the deprecate_selector_visible_attribute branch from b093bde to 369ea27 Compare January 9, 2022 11:48
@timhoffm
Copy link
Member
timhoffm commented Jan 9, 2022

It's reasonable to add get_visible() for standardization. I'm a bit torn with deprecating visible. As an alternative, we could turn it into a non-deprecated property. Reasons for not deprecating can be:

  • It's already there, so deprecating forces existing users to change. I'm not sure if that burden is justified/necessary in this case.
  • I have some vague considerations of more generally supporting properties in addition to setters+getters. In this case it would mean, selector.visible would be supported in addition to selector.get_visible() and selector.set_visible(). Advantage: It feels more pythonic and is more straight forward to read and write. Disadvantage: We introduce a second way of doing the same thing. I feel the advantage would be worth it, but it has not yet been discussed among devs.
    The thing is, if we go that way, it would be a bit annoying to deprecate visible only to reintroduce it later.

So overall, I'd recommend adding the property without deprecation for now. We can always deprecate later if we want to.

@ericpre
Copy link
Member Author
ericpre commented Jan 10, 2022

It's reasonable to add get_visible() for standardization. I'm a bit torn with deprecating visible. As an alternative, we could turn it into a non-deprecated property. Reasons for not deprecating can be:

  • It's already there, so deprecating forces existing users to change. I'm not sure if that burden is justified/necessary in this case.
  • I have some vague considerations of more generally supporting properties in addition to setters+getters. In this case it would mean, selector.visible would be supported in addition to selector.get_visible() and selector.set_visible(). Advantage: It feels more pythonic and is more straight forward to read and write. Disadvantage: We introduce a second way of doing the same thing. I feel the advantage would be worth it, but it has not yet been discussed among devs.
    The thing is, if we go that way, it would be a bit annoying to deprecate visible only to reintroduce it later.

This sounds good, but we have one issue here: visible=True and set_visible(True) doesn't do the same, as it is currently implemented.

  • visible=True only set the attribute
  • set_visible(True) set the visibility state of the selector and change the visibility state of all the selector artists.

This is the main reason that I went for deprecating the visible attribute, thinking that set_*/get_* is currently a widely pattern in matplotlib - sorry, I could have explained this when I created the PR.

So overall, I'd recommend adding the property without deprecation for now. We can always deprecate later if we want to.

We could keep the property, but I think that we should at least break its behaviour, because it currently can lead to inconsistent state of the selector:

import matplotlib.pyplot as plt
from matplotlib.widgets import RectangleSelector

fig, ax = plt.subplots()
selector = RectangleSelector(ax, lambda *args: None, interactive=True)
selector.extents = (0.4, 0.7, 0.3, 0.4)

assert selector.visible

selector.visible = False
print(f'is the selector visible? {selector.visible}')
for artist in selector.artists:
    print(f'is artist {artist} visible? {artist.get_visible()}')

@timhoffm
Copy link
Member
timhoffm commented Jan 10, 2022 •
edited
8000 Loading

We could keep the property, but I think that we should at least break its behaviour.

Agreed. I see it this way: visible is an internal state that was unfortunately made public. Since it is public now, and the reading part is working and there is no get_visible(), there are likely a number of users relying on that. Writing to visible does not well-behave. The non-breaking approach is:

  • add get_visible() as you have done to be consistent with the rest of the API

  • make visible a property.

    • The getter should not be deprecated.

    • For the setter we have two points to discuss
      a) do we want to allow setting in the future?
      b) do we want to immediately fix setting to behave like set_visible?

      IMHO there a two viable options:

      • b) yes, a) yes: This changes behavior by assuming what the user wants. It could be interpreted as fixing a bug. But if so, we can be permissive by not deprecating and forcing the user to change from one working solution to another.
      • b) no, a) no: If we don't dare to anticipate what the user intended, we should deprecate setting visible and require an explicit user action to switch to set_visible

@ericpre
Copy link
Member Author
ericpre commented Jan 11, 2022

IMHO there a two viable options:

  • b) yes, a) yes: This changes behavior by assuming what the user wants. It could be interpreted as fixing a bug. But if so, we can be permissive by not deprecating and forcing the user to change from one working solution to another.
  • b) no, a) no: If we don't dare to anticipate what the user intended, we should deprecate setting visible and require an explicit user action to switch to set_visible

Yes, I don't think that there other sensible options. I would prefer the second one. Option 1 can break user code: for example, when an user is changing the visible attribute but doesn't want the selector to be remove from the figure as it is currently possible. With option 2, the user will have time to adapt the code to the new behaviour.
I think that this is difficult to anticipate what the user intended to do, because there are always surprises! :)

@timhoffm
Copy link
Member

Agreed, option 2 it is then.

@ericpre ericpre force-pushed the deprecate_selector_visible_attribute branch from 369ea27 to c54a3b3 Compare January 12, 2022 18:31
@ericpre
Copy link
Member Author
ericpre commented Jan 13, 2022

@timhoffm, this is ready for review!

@QuLogic
Copy link
Member
QuLogic commented Jan 14, 2022

@timhoffm I thought you were hoping to add (or at least seemed to be fine with) more properties in widgets?

timhoffm
timhoffm reviewed Jan 20, 2022
View reviewed changes
@ericpre ericpre force-pushed the deprecate_selector_visible_attribute branch from c54a3b3 to d906c5c Compare April 6, 2022 19:08
@ericpre
Copy link
Member Author
ericpre commented Apr 7, 2022

Sorry for coming back on this only now. I rebased and deprecated the setter only, not the property.

@ericpre ericpre requested a review from timhoffm April 7, 2022 16:55
@QuLogic
Copy link
Member
QuLogic commented May 25, 2022

Ping @timhoffm

QuLogic
QuLogic reviewed May 25, 2022
View reviewed changes
timhoffm
timhoffm reviewed May 26, 2022
View reviewed changes
@tacaswell tacaswell added this to the v3.6.0 milestone May 27, 2022
@ericpre ericpre force-pushed the deprecate_selector_visible_attribute branch 2 times, most recently from 34e3a77 to f084ed2 Compare June 9, 2022 14:32
@ericpre ericpre requested review from QuLogic and timhoffm June 9, 2022 15:29
QuLogic
QuLogic reviewed Jun 21, 2022
View reviewed changes
lib/matplotlib/widgets.py Outdated
Comment on lines 2071 to 2072
"3.6", message="The visible setter was deprecated since %(since)s"
" and will be removed in %(removal)s. Use set_visible instead.")
Copy link
Member

Choose a reason for hiding this comment

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

No need to write the whole message; just set alternative="set_visible".

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, done!

@ericpre ericpre force-pushed the deprecate_selector_visible_attribute branch from f084ed2 to a2540ac Compare June 22, 2022 10:21
@QuLogic QuLogic merged commit 30b320a into matplotlib:main Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

59FB @timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
topic: widgets/UI
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@ericpre @timhoffm @QuLogic @tacaswell
0