Move integerness checks to SubplotSpec._from_subplot_args. by anntzer · Pull Request #17350 · matplotlib/matplotlib

anntzer · 2020-05-07T10:24:02Z

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

jklymak · 2020-05-07T14:57:05Z

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 · 2020-05-07T16:09:25Z

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

jklymak

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 · 2020-05-07T16:42:38Z

added the docstring changes.

QuLogic

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 · 2020-05-12T00:57:51Z

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

timhoffm

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 · 2020-05-18T19:38:44Z

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 · 2020-05-18T20:06:57Z

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

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 · 2020-05-18T20:55:58Z

oops

tacaswell · 2020-05-19T03:47:31Z

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 · 2020-05-19T07:58:20Z

that's fixed

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 added this to the v3.3.0 milestone May 7, 2020

anntzer mentioned this pull request May 7, 2020

FIX: allow start-stop subplot #17344

Closed

9 tasks

anntzer force-pushed the gscheckint branch 2 times, most recently from 7b4a234 to 5fb27ba Compare May 7, 2020 11:44

jklymak requested changes May 7, 2020

View reviewed changes

anntzer force-pushed the gscheckint branch from 5fb27ba to 3ae847d Compare May 7, 2020 16:42

jklymak approved these changes May 7, 2020

View reviewed changes

QuLogic requested changes May 12, 2020

View reviewed changes

QuLogic requested a review from timhoffm May 12, 2020 02:38

anntzer force-pushed the gscheckint branch from 3ae847d to 4337959 Compare May 17, 2020 20:59

timhoffm reviewed May 17, 2020

View reviewed changes

Comment thread lib/matplotlib/tests/test_figure.py

Comment thread lib/matplotlib/figure.py

anntzer force-pushed the gscheckint branch 3 times, most recently from a3b59c3 to deb7b6a Compare May 17, 2020 22:24

timhoffm approved these changes May 17, 2020

View reviewed changes

anntzer force-pushed the gscheckint branch 2 times, most recently from bf6cd0f to 912b184 Compare May 18, 2020 19:37

anntzer force-pushed the gscheckint branch from 912b184 to 01da990 Compare May 18, 2020 19:41

anntzer force-pushed the gscheckint branch from 01da990 to eff7337 Compare May 18, 2020 20:56

QuLogic approved these changes May 20, 2020

View reviewed changes

QuLogic merged commit 4a8aa8c into matplotlib:master May 20, 2020

anntzer deleted the gscheckint branch May 20, 2020 08:21

Uh oh!

Conversation

anntzer commented May 7, 2020 • edited by timhoffm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

jklymak commented May 7, 2020

Uh oh!

jklymak commented May 7, 2020

Uh oh!

jklymak left a comment

Choose a reason for hiding this comment

Uh oh!

anntzer commented May 7, 2020

Uh oh!

QuLogic left a comment

Choose a reason for hiding this comment

Uh oh!

jklymak commented May 12, 2020

Uh oh!

timhoffm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anntzer commented May 18, 2020

Uh oh!

timhoffm commented May 18, 2020

Uh oh!

anntzer commented May 18, 2020

Uh oh!

tacaswell commented May 19, 2020

Uh oh!

anntzer commented May 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

anntzer commented May 7, 2020 •

edited by timhoffm

Loading