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

ENH: Switch to a private, simpler AxesStack. #7377

Merged
merged 1 commit into from Aug 13, 2017

Conversation

Projects
None yet
7 participants
@anntzer
Contributor

anntzer commented Nov 2, 2016

attn @NelleV

The current implementation of AxesStack subclasses cbook.Stack, which
requires hashable keys, which leads to additional complexity on the
caller's side (_make_key).

Instead, switch to using two lists (keys and axes) and relying on
list.index, which makes the implementation much simpler.

Also make the new class private and deprecate the previous one.

(The original idea was to make AxesStack private to avoid having to deal with issues such as #7194. I originally thought that we could just replace it by an OrderedDict, which is still an option, but you need a really need a two-way dict to implement remove-by-value (instead of by key), so I ended up dropping that idea -- the benefit of dropping _make_key looked more enticing.)

@NelleV NelleV self-assigned this Nov 2, 2016

@NelleV

This comment has been minimized.

Show comment
Hide comment
@NelleV

NelleV Nov 2, 2016

Member

If you want a simpler PR at first, you can make one that deprecates the public AxesStack and "just" creates the private one. We can go from there in simplifing the API.
(else you'll have to fix the failing tests :) )

I'd wait on having the deprecation of the AxesStack vetted by others before moving forward. I think make AxesStack private is a good idea: it is only used in the figure object right now, in a private attribute, and probably aimed at being private from the start.
attn @efiring @tacaswell

Member

NelleV commented Nov 2, 2016

If you want a simpler PR at first, you can make one that deprecates the public AxesStack and "just" creates the private one. We can go from there in simplifing the API.
(else you'll have to fix the failing tests :) )

I'd wait on having the deprecation of the AxesStack vetted by others before moving forward. I think make AxesStack private is a good idea: it is only used in the figure object right now, in a private attribute, and probably aimed at being private from the start.
attn @efiring @tacaswell

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Nov 2, 2016

Member

👍 in principle but we should try to support as much of the old API as possible.

I know I have written SO answers which reached in and touched the AxesStack....

The balance against making everything private is that if you do not give users enough public knobs to get done what they need to get done, they will find the private knobs and just do it. Once enough user are using a private API it become defacto public and we are as bad off or worse than we would have been other wise.

Member

tacaswell commented Nov 2, 2016

👍 in principle but we should try to support as much of the old API as possible.

I know I have written SO answers which reached in and touched the AxesStack....

The balance against making everything private is that if you do not give users enough public knobs to get done what they need to get done, they will find the private knobs and just do it. Once enough user are using a private API it become defacto public and we are as bad off or worse than we would have been other wise.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Nov 2, 2016

@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Nov 2, 2016

Contributor

@NelleV Hopefully everything should be fixed now (I didn't realize that I need to keep track both of the axes stack and of the order in which the axes were created).

@tacaswell The only reference to AxesStack I could find on SO is http://stackoverflow.com/questions/24104990/how-to-get-a-list-of-axes-for-a-figure-in-pyplot which somehow ironically ends with your comment

fig is a Figure object. I am not sure where AxesStack gets used, but you should probably not be using it.

In any case this PR literally doesn't change any public API -- the old AxesStack class is still there, it's just used nowhere :-) (and 1. the AxesStack object instantiated by the Figure class was a private attribute, so it's fair game to change it and 2. in fact, the new private AxesStack implements (nearly) the same API as the old one...).

Honestly if we start having to worry about modifying private APIs then I don't know what's left to do other than adding features over features and never fixing old stuff because, well, it may always break someone else who is playing with the internals (that person should have asked for a public API, or known better). Again, I'm not removing AxesStack for now; if someone complains that they were actually using it, we can always consider undeprecating it.

Contributor

anntzer commented Nov 2, 2016

@NelleV Hopefully everything should be fixed now (I didn't realize that I need to keep track both of the axes stack and of the order in which the axes were created).

@tacaswell The only reference to AxesStack I could find on SO is http://stackoverflow.com/questions/24104990/how-to-get-a-list-of-axes-for-a-figure-in-pyplot which somehow ironically ends with your comment

fig is a Figure object. I am not sure where AxesStack gets used, but you should probably not be using it.

In any case this PR literally doesn't change any public API -- the old AxesStack class is still there, it's just used nowhere :-) (and 1. the AxesStack object instantiated by the Figure class was a private attribute, so it's fair game to change it and 2. in fact, the new private AxesStack implements (nearly) the same API as the old one...).

Honestly if we start having to worry about modifying private APIs then I don't know what's left to do other than adding features over features and never fixing old stuff because, well, it may always break someone else who is playing with the internals (that person should have asked for a public API, or known better). Again, I'm not removing AxesStack for now; if someone complains that they were actually using it, we can always consider undeprecating it.

@NelleV

This comment has been minimized.

Show comment
Hide comment
@NelleV

NelleV Nov 10, 2016

Member

The code looks good to me.
I don't think we loose any functionality with this switch and the code is much cleaner.

Member

NelleV commented Nov 10, 2016

The code looks good to me.
I don't think we loose any functionality with this switch and the code is much cleaner.

@efiring

This comment has been minimized.

Show comment
Hide comment
@efiring

efiring Nov 10, 2016

Member

Also related to #7335.

Member

efiring commented Nov 10, 2016

Also related to #7335.

@QuLogic

Maybe a silly question, but is the only difference between this and an OrderedDict that it can do operations based on the value instead of the key? A comment/doc to that effect would be useful for the future.

Show outdated Hide outdated lib/matplotlib/figure.py
def as_list(self):
"""Copy of the list of axes, in the order of insertion.
"""
return [ax for _, _, ax in sorted(self._items)]

This comment has been minimized.

@QuLogic

QuLogic Nov 11, 2016

Member

I know it's originally sorted this way, but I wonder if that's even necessary...

@QuLogic

QuLogic Nov 11, 2016

Member

I know it's originally sorted this way, but I wonder if that's even necessary...

This comment has been minimized.

@anntzer

anntzer Nov 11, 2016

Contributor

The "keep creation order" behavior is mandated by the tests right now, and I don't see a reason to get rid of it.

@anntzer

anntzer Nov 11, 2016

Contributor

The "keep creation order" behavior is mandated by the tests right now, and I don't see a reason to get rid of it.

@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Nov 11, 2016

Contributor

@QuLogic also because keys may not be hashable. Added a comment to that effect.

Contributor

anntzer commented Nov 11, 2016

@QuLogic also because keys may not be hashable. Added a comment to that effect.

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Nov 11, 2016

Member

@anntzer It as always fun when 2.5 year old comments come back at me ;)

Member

tacaswell commented Nov 11, 2016

@anntzer It as always fun when 2.5 year old comments come back at me ;)

@NelleV

This comment has been minimized.

Show comment
Hide comment
@NelleV

NelleV Nov 11, 2016

Member

This PR clearly supersedes #7335, thought I think the latter is 2.0 safe while this one isn't.

Member

NelleV commented Nov 11, 2016

This PR clearly supersedes #7335, thought I think the latter is 2.0 safe while this one isn't.

def get(self, key):
"""Find the axes corresponding to a key; defaults to `None`.
"""
return next((ax for _, k, ax in self._items if k == key), None)

This comment has been minimized.

@tacaswell

tacaswell Nov 11, 2016

Member

@anntzer I consistently learn new python details from your code.

@tacaswell

tacaswell Nov 11, 2016

Member

@anntzer I consistently learn new python details from your code.

"""
# Skipping existing Axes is needed to support calling `add_axes` with
# an already existing Axes.
if not any(a == ax for _, _, a in self._items):

This comment has been minimized.

@tacaswell

tacaswell Nov 11, 2016

Member

This probably needs to handle the duplicate key case?

@tacaswell

tacaswell Nov 11, 2016

Member

This probably needs to handle the duplicate key case?

This comment has been minimized.

@anntzer

anntzer Nov 11, 2016

Contributor

Each call to add is actually preceded by a call to get, so technically we can rely on the caller code to never try to add in a duplicate key. Also I worded the docstring carefully: the pair is not appended if the axes is already present but there's no check on whether the key is :-)

In practice I could make the behavior (no checking for duplicate key) clearer in the docstring, or add a check and error out if the key is already there; either is fine with me. Please let me know your preference.

@anntzer

anntzer Nov 11, 2016

Contributor

Each call to add is actually preceded by a call to get, so technically we can rely on the caller code to never try to add in a duplicate key. Also I worded the docstring carefully: the pair is not appended if the axes is already present but there's no check on whether the key is :-)

In practice I could make the behavior (no checking for duplicate key) clearer in the docstring, or add a check and error out if the key is already there; either is fine with me. Please let me know your preference.

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Nov 11, 2016

Member

👍 One small concern about the duplicate key, other wise LGTM

I strongly prefer to not backport this for 2.0.

Member

tacaswell commented Nov 11, 2016

👍 One small concern about the duplicate key, other wise LGTM

I strongly prefer to not backport this for 2.0.

@NelleV

This comment has been minimized.

Show comment
Hide comment
@NelleV

NelleV Nov 11, 2016

Member

We should never have duplicate keys, so what should be the expected behaviour? Raise an error?

Member

NelleV commented Nov 11, 2016

We should never have duplicate keys, so what should be the expected behaviour? Raise an error?

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Nov 11, 2016

Member

Lets go with raise (and keep the raise / API change in #7335).

Member

tacaswell commented Nov 11, 2016

Lets go with raise (and keep the raise / API change in #7335).

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Nov 11, 2016

Member

Hmm, so I just thought of something that the key function actually handles that isn't here, namely, that elements of args and kwargs may be mutable, which may not be something you want in the key. For example:

rect = np.array([0.2, 0.2, 0.2, 0.2])
ax1 = fig.add_axes(rect, ...)
rect[:2] += 0.2
ax2 = fig.add_axes(rect, ...)

ax2 should be new, but I think it will return ax1 with this class.

Member

QuLogic commented Nov 11, 2016

Hmm, so I just thought of something that the key function actually handles that isn't here, namely, that elements of args and kwargs may be mutable, which may not be something you want in the key. For example:

rect = np.array([0.2, 0.2, 0.2, 0.2])
ax1 = fig.add_axes(rect, ...)
rect[:2] += 0.2
ax2 = fig.add_axes(rect, ...)

ax2 should be new, but I think it will return ax1 with this class.

@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Nov 11, 2016

Contributor

In fact I just realized there's a much bigger issue here: you can't use == to compare arrays for full equality, so something as simple as

plt.axes(np.array(...)); plt.axes(np.array(...))

will fail with

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

I guess there's no way around having some variant of make_key :-(

Contributor

anntzer commented Nov 11, 2016

In fact I just realized there's a much bigger issue here: you can't use == to compare arrays for full equality, so something as simple as

plt.axes(np.array(...)); plt.axes(np.array(...))

will fail with

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

I guess there's no way around having some variant of make_key :-(

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Nov 11, 2016

Member

do we need to support mutable/non-hashable keys?

Member

tacaswell commented Nov 11, 2016

do we need to support mutable/non-hashable keys?

@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Nov 11, 2016

Contributor

I would like to claim no, of course... in fact I would even like to claim that the whole

    If the figure already has an axes with the same parameters,
    then it will simply make that axes current and return it.

behavior is seriously misguided.

But I've learnt better than to propose breaking compatibility here :-)

As a side note, the docstring of Figure.add_axes continue with

    If you do not want this behavior, e.g., you want to force the
    creation of a new Axes, you must use a unique set of args and
    kwargs.  The axes :attr:`~matplotlib.axes.Axes.label`
    attribute has been exposed for this purpose.  e.g., if you want
    two axes that are otherwise identical to be added to the
    figure, make sure you give them unique labels::

        fig.add_axes(rect, label='axes1')
        fig.add_axes(rect, label='axes2')

But something like

ax_foo = fig.add_axes(rect); ax_foo.set_label("foo")
ax_bar = fig.add_axes(rect); ax_bar.set_label("bar")

doesn't create two separate axes (which is unsurprising when you know the implementation, and technically follows the docs, but rather counterintuitive from a user POV).

Contributor

anntzer commented Nov 11, 2016

I would like to claim no, of course... in fact I would even like to claim that the whole

    If the figure already has an axes with the same parameters,
    then it will simply make that axes current and return it.

behavior is seriously misguided.

But I've learnt better than to propose breaking compatibility here :-)

As a side note, the docstring of Figure.add_axes continue with

    If you do not want this behavior, e.g., you want to force the
    creation of a new Axes, you must use a unique set of args and
    kwargs.  The axes :attr:`~matplotlib.axes.Axes.label`
    attribute has been exposed for this purpose.  e.g., if you want
    two axes that are otherwise identical to be added to the
    figure, make sure you give them unique labels::

        fig.add_axes(rect, label='axes1')
        fig.add_axes(rect, label='axes2')

But something like

ax_foo = fig.add_axes(rect); ax_foo.set_label("foo")
ax_bar = fig.add_axes(rect); ax_bar.set_label("bar")

doesn't create two separate axes (which is unsurprising when you know the implementation, and technically follows the docs, but rather counterintuitive from a user POV).

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Nov 11, 2016

Member

Was my example not convincing enough (not that I actually use it)? Maybe not necessarily in that use exactly, but using a NumPy array doesn't seem that uncommon.

Member

QuLogic commented Nov 11, 2016

Was my example not convincing enough (not that I actually use it)? Maybe not necessarily in that use exactly, but using a NumPy array doesn't seem that uncommon.

@efiring

This comment has been minimized.

Show comment
Hide comment
@efiring

efiring Nov 11, 2016

Member

On 2016/11/11 8:23 AM, Antony Lee wrote:

I would like to claim no, of course... in fact I would even like to
claim that the whole

|If the figure already has an axes with the same parameters, then it will
simply make that axes current and return it. |

behavior is seriously misguided.

I suspect we all agree on this. I think it is the result of starting
off following Matlab long ago for the sake of compatibility. Then our
code and documentation get ever more tied in knots as we go beyond
Matlab while trying to maintain compatibility with a bad API. Maybe we
need to be a little more willing to shed such relics. How much user
code is likely to rely on this?

Member

efiring commented Nov 11, 2016

On 2016/11/11 8:23 AM, Antony Lee wrote:

I would like to claim no, of course... in fact I would even like to
claim that the whole

|If the figure already has an axes with the same parameters, then it will
simply make that axes current and return it. |

behavior is seriously misguided.

I suspect we all agree on this. I think it is the result of starting
off following Matlab long ago for the sake of compatibility. Then our
code and documentation get ever more tied in knots as we go beyond
Matlab while trying to maintain compatibility with a bad API. Maybe we
need to be a little more willing to shed such relics. How much user
code is likely to rely on this?

@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Nov 11, 2016

Contributor

If you're willing to break backcompatibility here, the good thing is that's there's an obvious deprecation path:

  1. merge #7335 as a stopgap measure and 2) raise a deprecation warning whenever a key collision occurs
    A few (cough... one... cough) releases later,
  2. entirely drop that behavior (i.e. create a new axes every time).

By the way I assume(?) it is possible to change the rectangle where an axes sit, in which case that provides another way to break the current implementation (create an axes, move it somewhere else, and either try to create another axes where the first one was, or try to create another axes where the first one is currently).

In fact even MATLAB doesn't provide that behavior:

>> axes('Position', [.1 .1 .7 .7]); axes('Position', [.1, .1, .7, .7]); get(gcf, 'children')

ans = 

  2×1 Axes array:

  Axes
  Axes
Contributor

anntzer commented Nov 11, 2016

If you're willing to break backcompatibility here, the good thing is that's there's an obvious deprecation path:

  1. merge #7335 as a stopgap measure and 2) raise a deprecation warning whenever a key collision occurs
    A few (cough... one... cough) releases later,
  2. entirely drop that behavior (i.e. create a new axes every time).

By the way I assume(?) it is possible to change the rectangle where an axes sit, in which case that provides another way to break the current implementation (create an axes, move it somewhere else, and either try to create another axes where the first one was, or try to create another axes where the first one is currently).

In fact even MATLAB doesn't provide that behavior:

>> axes('Position', [.1 .1 .7 .7]); axes('Position', [.1, .1, .7, .7]); get(gcf, 'children')

ans = 

  2×1 Axes array:

  Axes
  Axes
return self._axstack.as_list()
axes = property(fget=_get_axes, doc="Read-only: list of axes in Figure")
axes = property(lambda self: self._axstack.as_list(),

This comment has been minimized.

@Kojoley

Kojoley Nov 13, 2016

Member

Can't this be

    @property
    def axes(self):
        """Read-only: list of axes in Figure"""
        return self._axstack.as_list()
@Kojoley

Kojoley Nov 13, 2016

Member

Can't this be

    @property
    def axes(self):
        """Read-only: list of axes in Figure"""
        return self._axstack.as_list()

This comment has been minimized.

@anntzer

anntzer Nov 13, 2016

Contributor

I don't think one is more readable that the other?

@anntzer

anntzer Nov 13, 2016

Contributor

I don't think one is more readable that the other?

@NelleV

This comment has been minimized.

Show comment
Hide comment
@NelleV

NelleV Jan 17, 2017

Member

Is there any interest in pushing this forward?

Member

NelleV commented Jan 17, 2017

Is there any interest in pushing this forward?

@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Jan 17, 2017

Contributor

Why not :-)
Rebased, and updated the deprecation warning to 2.1.

Contributor

anntzer commented Jan 17, 2017

Why not :-)
Rebased, and updated the deprecation warning to 2.1.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jan 17, 2017

Current coverage is 62.01% (diff: 97.05%)

Merging #7377 into master will decrease coverage by 0.09%

@@             master      #7377   diff @@
==========================================
  Files           174        174          
  Lines         56065      56068     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          34820      34770    -50   
- Misses        21245      21298    +53   
  Partials          0          0          

Powered by Codecov. Last update fa95b58...9b7bfdc

codecov-io commented Jan 17, 2017

Current coverage is 62.01% (diff: 97.05%)

Merging #7377 into master will decrease coverage by 0.09%

@@             master      #7377   diff @@
==========================================
  Files           174        174          
  Lines         56065      56068     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          34820      34770    -50   
- Misses        21245      21298    +53   
  Partials          0          0          

Powered by Codecov. Last update fa95b58...9b7bfdc

@NelleV

NelleV approved these changes Jan 20, 2017

I think this looks good. I'd rather merge this sooner than later, if something is terribly wrong, we might pick it up before the 2.1 release.

@NelleV NelleV changed the title from Switch to a private, simpler AxesStack. to [MRG+1] Switch to a private, simpler AxesStack. Jan 20, 2017

@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Jan 20, 2017

Contributor

Actually, see the discussion starting at #7377 (comment). I think the issue mentioned by @QuLogic cannot really be fixed with the current implementation so perhaps we should instead follow the path mentioned in #7377 (comment) (see comment by @efiring re: backcompatibility, too).

Contributor

anntzer commented Jan 20, 2017

Actually, see the discussion starting at #7377 (comment). I think the issue mentioned by @QuLogic cannot really be fixed with the current implementation so perhaps we should instead follow the path mentioned in #7377 (comment) (see comment by @efiring re: backcompatibility, too).

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Aug 10, 2017

Member

This needs a rebase.

Member

tacaswell commented Aug 10, 2017

This needs a rebase.

Switch to a private, simpler AxesStack.
The current implementation of AxesStack subclasses cbook.Stack, which
requires hashable keys, which leads to additional complexity on the
caller's side (`_make_key`).

Instead, switch to using two lists (keys and axes) and relying on
`list.index`, which makes the implementation much simpler.

Also make the new class private and deprecate the previous one.
@anntzer

This comment has been minimized.

Show comment
Hide comment
@anntzer

anntzer Aug 10, 2017

Contributor

done

Contributor

anntzer commented Aug 10, 2017

done

@tacaswell tacaswell changed the title from [MRG+1] Switch to a private, simpler AxesStack. to ENH: Switch to a private, simpler AxesStack. Aug 13, 2017

@tacaswell tacaswell merged commit 4c33d97 into matplotlib:master Aug 13, 2017

7 of 8 checks passed

codecov/project/tests 98.71% (-0.01%) compared to b13c68a
Details
ci/circleci: docs-python27 Your tests passed on CircleCI!
Details
ci/circleci: docs-python35 Your tests passed on CircleCI!
Details
codecov/patch 91.17% of diff hit (target 50%)
Details
codecov/project/library 60.22% (target 50%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Aug 13, 2017

Member

Are we not worried about #7377 (comment)?

Member

QuLogic commented Aug 13, 2017

Are we not worried about #7377 (comment)?

@anntzer anntzer deleted the anntzer:private-axesstack branch Aug 13, 2017

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Aug 14, 2017

Member

@QuLogic I missed that when I was re-reading the comments last night, sorry.

Member

tacaswell commented Aug 14, 2017

@QuLogic I missed that when I was re-reading the comments last night, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment