Skip to content

Deprecate empty offsets in get_path_collection_extents #23200

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

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

oscargus
Copy link
Member

@oscargus oscargus commented Jun 3, 2022

PR Summary

Closes #1735

Based on the issue, the outcome of an empty offsets is not well defined. This is now deprecated.

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

@timhoffm
Copy link
Member

timhoffm commented Jun 4, 2022

Running the tests locally does not seem to break anything, but there may be image comparisons tests that fails because of this

Codecov says your addition is never exercised in the tests; so it cannot break the tests 😄 .

timhoffm
timhoffm previously approved these changes Jun 4, 2022
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Anyway, I wouldn't know what to test. Passing empty offsets is trivial and almost not worth a test. But maybe we should do that to pin this as intended behavior.

@oscargus
Copy link
Member Author

oscargus commented Jun 4, 2022

What I meant was that we do not have any tests that relies on being able to pass an empty offsets (nor an empty paths). And, as it turned out, no in the additional ones on the CI (as I may not have all optional dependencies installed).

The problem I think is to know if it ever is useful to pass an empty offsets or if some valid input data would pass an empty offsets. Testing the function directly doesn't really make sense just to exercise the code path, but one would probably like to do it from an actual user-level API (even though this technically is one).

@timhoffm
Copy link
Member

timhoffm commented Jun 6, 2022

If we are not sure that somebody is relying on the current but ambiguous behavior, we should deprecate it instead of immediately erroring out. That gives people time to object. The issue has been standing since 2013, so there is no rush in turning it into an error.

@timhoffm timhoffm dismissed their stale review June 6, 2022 22:36

Be defensive and deprecate first.

@oscargus oscargus added this to the v3.6.0 milestone Jun 11, 2022
@oscargus oscargus changed the title Error on empty offsets in get_path_collection_extents Deprecate empty offsets in get_path_collection_extents Jun 11, 2022
@anntzer
Copy link
Contributor

anntzer commented Jun 11, 2022

Perhaps what this should really return is a null bbox (see discussion at #17115 (comment))? Although we can always change that later if we manage to resolve #17115 at some point.

@oscargus oscargus marked this pull request as draft July 7, 2022 07:19
@oscargus oscargus removed this from the v3.6.0 milestone Aug 18, 2022
@oscargus oscargus marked this pull request as ready for review March 1, 2023 11:14
@oscargus oscargus added this to the v3.8.0 milestone Mar 1, 2023
@oscargus
Copy link
Member Author

oscargus commented Mar 1, 2023

Not sure why I put this as draft, but rebased and updated deprecation version.

@ksunden ksunden merged commit 304944e into matplotlib:main Mar 13, 2023
@oscargus oscargus deleted the pathfix branch March 13, 2023 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_path.get_path_collection_extents potentially wrong return value
5 participants