Skip to content

Move integerness checks to SubplotSpec._from_subplot_args. #17350

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 1 commit into from
May 20, 2020
Merged

Move integerness checks to SubplotSpec._from_subplot_args. #17350

merged 1 commit into from
May 20, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented May 7, 2020 •
edited by timhoffm
Loading

This avoids having to later do them same for other places (axes_grid)
which rely on the same machinery to convert three-digit specifications.

I'll assume that the already existing deprecation note for add_subplot
is effectively sufficient.

Alternative for #17344; only one of them is RC, of course.

Closes #17343.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. API: argument checking labels May 7, 2020
@anntzer anntzer added this to the v3.3.0 milestone May 7, 2020
@anntzer anntzer mentioned this pull request May 7, 2020
Closed
9 tasks
@anntzer anntzer force-pushed the gscheckint branch 2 times, most recently from 7b4a234 to 5fb27ba Compare May 7, 2020 11:44
@jklymak
Copy link
Member

jklymak commented May 7, 2020

I agree that this is nicer to not re-do the checks in two spots, but I feel you've gone a few steps too far here.

Reading the code I'd have no idea where these checks were going to be done. I think burying them in ax = subplot_class_factory(projection_class)(self, *args, **kwargs) is not a good architecture.

Also do all the other subplot classes have these checks? If not, they now no longer have them, whereas before they did...

Can we instead just add a helper to SubplotSpec that gets called twice?

@jklymak
Copy link
Member

jklymak commented May 7, 2020

.. see a revised #17344 for a refactor along these lines, but I think a little more clear, if redundant.

jklymak
jklymak requested changes May 7, 2020
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

I see, SubplotSpec._from_subplot_args gets called in the init to subplotBase, and all subplots have this base from the factory, so this change is OK.

However, we need to include the documentation changes from #17344. I don't think this feature should be undocumented.

@anntzer
Copy link
Contributor Author

anntzer commented May 7, 2020

added the docstring changes.

QuLogic
QuLogic requested changes May 12, 2020
View reviewed changes
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

I understand the benefit of this, but moving the normalization to after _process_projection_requirements (and thus _make_key) means that the key of the Axes is inconsistent. This means that fig.add_subplot(111) and fig.add_subplot(1, 1, 1) are no longer the same, i.e., on master:

>>> import matplotlib.pyplot as plt
>>> fig = plt.figure()
>>> ax0 = fig.add_subplot(111)
>>> ax1 = fig.add_subplot(1, 1, 1)
__main__:1: MatplotlibDeprecationWarning: Adding an axes using the same arguments as a previous axes currently reuses the earlier instance.  In a future version, a new instance will always be created and returned.  Meanwhile, this warning can be suppressed, and the future behavior ensured, by passing a unique label to each axes instance.
>>> ax0 is ax1
True

but now:

>>> import matplotlib.pyplot as plt
>>> fig = plt.figure()
>>> ax0 = fig.add_subplot(111)
>>> ax1 = fig.add_subplot(1, 1, 1)
>>> ax0
<matplotlib.axes._subplots.AxesSubplot object at 0x7f22cb92b8d0>
>>> ax1
<matplotlib.axes._subplots.AxesSubplot object at 0x7f22cb537d68>
>>> ax0 is ax1
False

@jklymak
Copy link
Member

jklymak commented May 12, 2020

I think thats fine. We are trying to deprecate the key behaviour anyway.

@QuLogic QuLogic requested a review from timhoffm May 12, 2020 02:38
timhoffm
timhoffm reviewed May 17, 2020
View reviewed changes
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.

I genuinely don't like add_subplot(n, m, (j, k)), but of course we shouldn't break it.

This seems reasonable; modulo the comments.

@anntzer anntzer force-pushed the gscheckint branch 3 times, most recently from a3b59c3 to deb7b6a Compare May 17, 2020 22:24
@anntzer anntzer force-pushed the gscheckint branch 2 times, most recently from bf6cd0f to 912b184 Compare May 18, 2020 19:37
@anntzer
Copy link
Contributor Author

anntzer commented May 18, 2020

Fixed @QuLogic's concern by adding a single conversion of 3-digit integers to (i, j, k) -- any invalid value will just pass through and still trigger error handling via SubplotSpec._from_subplot_args.

@timhoffm
Copy link
Member

This generates numerous warnings like this in the doc build:

WARNING: /home/circleci/project/examples/subplots_axes_and_figures/axes_margins.py failed to execute correctly: Traceback (most recent call last):
  File "/home/circleci/.local/lib/python3.6/site-packages/sphinx_gallery/gen_rst.py", line 460, in _memory_usage
    out = func()
  File "/home/circleci/.local/lib/python3.6/site-packages/sphinx_gallery/gen_rst.py", line 442, in __call__
    exec(self.code, self.fake_main.__dict__)
  File "/home/circleci/project/examples/subplots_axes_and_figures/axes_margins.py", line 24, in <module>
    ax1 = plt.subplot(212)
  File "/home/circleci/project/lib/matplotlib/pyplot.py", line 1074, in subplot
    ax = fig.add_subplot(*args, **kwargs)
  File "/home/circleci/project/lib/matplotlib/figure.py", line 1401, in add_subplot
    ax = subplot_class_factory(projection_class)(self, *args, **kwargs)
  File "/home/circleci/project/lib/matplotlib/axes/_subplots.py", line 36, in __init__
    self._subplotspec = SubplotSpec._from_subplot_args(fig, args)
  File "/home/circleci/project/lib/matplotlib/gridspec.py", line 691, in _from_subplot_args
    raise TypeError(f"subplot() takes 1 or 3 positional arguments but "
TypeError: subplot() takes 1 or 3 positional arguments but 0 were given

@anntzer
Move integerness checks to SubplotSpec._from_subplot_args.
eff7337 
This avoids having to later do them same for other places (axes_grid)
which rely on the same machinery to convert three-digit specifications.

I'll assume that the already existing deprecation note for add_subplot
is effectively sufficient.
@anntzer
Copy link
Contributor Author

anntzer commented May 18, 2020

oops

@tacaswell
Copy link
Member

We talked about this on the weekly call and after some discussion settled on restoring fig.add_subplot(1, 1, 1) and fig.add_subplot(111) returning the same instance. We may not like the behavior of returning an existing Axes, but it only doing in sometimes is worse.

@anntzer
Copy link
Contributor Author

anntzer commented May 19, 2020

that's fixed

@QuLogic QuLogic merged commit 4a8aa8c into matplotlib:master May 20, 2020
@anntzer anntzer deleted the gscheckint branch May 20, 2020 08:21
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

@jklymak jklymak jklymak approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
API: argument checking Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.

Regression in add_subplot..
5 participants
@anntzer @jklymak @timhoffm @tacaswell @QuLogic