FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Expire deprecations in cbook.deprecation by oscargus · Pull Request #22514 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Expire deprecations in cbook.deprecation - #22514

Merged
QuLogic merged 1 commit into
matplotlib:mainfrom
oscargus:cbookdeprecationremoval
Feb 25, 2022
ConversationCommits (1)ChecksFiles changed
Merged

Expire deprecations in cbook.deprecation#22514
QuLogic merged 1 commit into
matplotlib:mainfrom
oscargus:cbookdeprecationremoval

Conversation

oscargus commented Feb 20, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

Expire deprecations in cbook.deprecation. Will add a release note, but was a bit confused which things actually are deprecated.

cbook.MatplotlibDeprecationWarning and cbook.mplDeprecation are deprecated based on the earlier release note:

The module matplotlib.cbook.deprecation is considered internal and will be removed from the public API. This also holds for deprecation-related re-imports in matplotlib.cbook, i.e. matplotlib.cbook.deprecated(), matplotlib.cbook.warn_deprecated(), matplotlib.cbook.MatplotlibDeprecationWarning and matplotlib.cbook.mplDeprecation.
If needed, external users may import MatplotlibDeprecationWarning directly from the matplotlib namespace. mplDeprecation is only an alias of MatplotlibDeprecationWarning and should not be used anymore.

However, it is possible to import cbook.MatplotlibDeprecationWarning and cbook.mplDeprecation and use them without any deprecation warnings. Is it still OK to removed them?

Btw, the current code gave a dual deprecation warning: :-)

In [4]: from matplotlib.collections import LineCollection

In [5]: LineCollection(1, 2)
C:\Users\Oscar\AppData\Local\Temp\ipykernel_21208\127307086.py:1: MatplotlibDeprecationWarning: 
The warn_deprecated function was deprecated in Matplotlib 3.4 and will be removed two minor releases later.
  LineCollection(1, 2)
C:\Users\Oscar\AppData\Local\Temp\ipykernel_21208\127307086.py:1: MatplotlibDeprecationWarning: Since 3.4, passing LineCollection arguments other than the first, 'segments', as positional arguments is deprecated, and they will become keyword-only arguments two minor releases later.
  LineCollection(1, 2)

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.
  • 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).

oscargus force-pushed the cbookdeprecationremoval branch from c2e06ae to fb3a50e Compare February 20, 2022 16:07
oscargus added API: changes Changes to the public API, typically requiring deprecation. Maintenance labels Feb 20, 2022
oscargus marked this pull request as draft February 20, 2022 16:54

oscargus commented Feb 20, 2022

Copy link
Copy Markdown
Member Author

The failing tests are an indication that these were not properly deprecated earlier (and that I didn't look carefully enough across the entire code base...).

Should they still be removed or should they be properly deprecated now instead?

timhoffm commented Feb 21, 2022

Copy link
Copy Markdown
Member

I don't think we have or easily could have added a warning on importing the class via cbook.MatplotlibDeprecationWarning atbruntime. But we have properly documented the deprecation: https://matplotlib.org/devdocs/api/prev_api_changes/api_changes_3.4.0.html#deprecation-related-functionality-is-considered-internal

Also it's rather unlikely for end users to import MatplotlibDeprecationWarning. So let's continue with the removal as planned.

The internal import errors are an oversight and should be fixed.

oscargus force-pushed the cbookdeprecationremoval branch from fb3a50e to 6f621c1 Compare February 24, 2022 17:55
oscargus marked this pull request as ready for review February 24, 2022 17:57
oscargus force-pushed the cbookdeprecationremoval branch from 6f621c1 to ebc9d65 Compare February 24, 2022 18:17

oscargus commented Feb 24, 2022

Copy link
Copy Markdown
Member Author

I've fixed the failing tests and added release note. (Azure is not that happy, but I doubt it is my fault.)

QuLogic merged commit 3a994d2 into matplotlib:main Feb 25, 2022
oscargus deleted the cbookdeprecationremoval branch February 25, 2022 05:32
oscargus added this to the v3.6.0 milestone Aug 1, 2022

mwaskom commented Aug 23, 2022

Copy link
Copy Markdown

This removal hard breaks a seaborn function.

Also it's rather unlikely for end users to import MatplotlibDeprecationWarning.

Curious what the basis for this claim was? https://github.com/search?q=%22from+matplotlib.cbook+import+MatplotlibDeprecationWarning%22&type=Code shows 2k+ direct imports of this class, and searching for alternate usage patterns turns up others as well.

I don't think we have or easily could have added a warning on importing the class

I agree that this would have been hard! But IMO that puts the burden on matplotlib to deprecate with a bit more care (e.g., proactively searching for usage in downstream libraries, etc.). You are right that this was "properly documented", but it is mentioned in the middle of a very long list of API changes (there are >4000 words on https://matplotlib.org/stable/api/prev_api_changes/api_changes_3.4.0.html). Downstream libraries have the reasonable expectation that they can rely on unit test coverage to surface deprecation warnings, since matplotlib is usually good about that sort of thing.

In the future, please boost the signal of any silent deprecations.

tacaswell commented Aug 23, 2022

Copy link
Copy Markdown
Member

We should back out the change that broke seaborn and sort out a way to loudly complain. This is a blocking issue for mpl36.

tacaswell mentioned this pull request Aug 23, 2022
Closed

oscargus commented Aug 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Assuming that it is primarily the import of MatlabDeprecationWarning from cbook that is required (and that importing cbook.deprecation is not required), it should at least be pretty straightforward to get it up and running again. Making it scream loud is another problem (which I believe more classes suffer from).

QuLogic commented Aug 23, 2022

Copy link
Copy Markdown
Member

Making it scream loud is another problem (which I believe more classes suffer from).

This was not possible in 3.4 as it supported Python 3.6; it is possible now with __getattr__, for which we already have a helper.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

QuLogic QuLogic approved these changes
timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

API: changes Changes to the public API, typically requiring deprecation. Maintenance

Projects

None yet

Milestone

v3.6.0

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL