Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Single axes artist #3835

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 4 commits into from
Dec 13, 2014
Merged

Single axes artist #3835

merged 4 commits into from
Dec 13, 2014

Conversation

tacaswell
Copy link
Member

This adds check to ensure that you don't add an artist to more than one axes (which does not work because the transform stacks clobber each other).

@jenshnielsen
Copy link
Member

👍 Looks like the demo_colorbar_with_inset_locator.py example fails with this change. There is probably need for a modification of axis_grid1

@jenshnielsen
Copy link
Member

I think this resolves #3776

@tacaswell
Copy link
Member Author

I suspect that is has broken a whole bunch of the axes_grid stuff. Don't really understand that error on first pass, my guess is that the base class __init__ function is not being called...

@tacaswell
Copy link
Member Author

Well, now a different example is broken but for a different reason so that is progress...

ACCEPTS: an :class:`~matplotlib.axes.Axes` instance
"""
warnings.warn(_get_axes_msg, mplDeprecation, stacklevel=1)
raise Exception("die to find usage")
Copy link
Member

Choose a reason for hiding this comment

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

Still here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that should not be there...

@tacaswell
Copy link
Member Author

@pelson @efiring can this get a yay/nay?

I can be convinced to put off deprecating the {get,set}_axes methods.

# call the get method on the parents' property
return Artist.axes.fget(self)

@axes.setter
Copy link
Member

Choose a reason for hiding this comment

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

Your re-definition of the axes property above looks unnecessary. I think you could omit it, and then replace the @axes.setter decorator with @Artist.axes.setter.

@efiring
Copy link
Member

efiring commented Dec 12, 2014

The idea of blocking a change in the axes seems good, but I don't have a feel for other implications of the implementation via a switch to properties. What are the other attributes for which you want to abolish the getters and setters? In general, substituting properties seems like a way to make the interface more streamlined and pythonic, but it's a big change in style--although we have snuck in a few properties before.
Overall, I'm inclined to press the button and see what happens; I suspect you have a perfectly good strategy in mind.

@tacaswell
Copy link
Member Author

I now remember why I was trying to get rid of the getter/setters. They are what are scraped for the docs to generate the ACCEPTS tables. By removing the set_axes method axes will no longer show up in the tables but would be accepted as a kwargs anyway (via secret non-documented features, which now that I type it is also a problem).

The other (immediate) candidates for this are the the rest of the things that should not be in that table.

@efiring
Copy link
Member

efiring commented Dec 12, 2014

Failure on 2.6 looks like an unrelated svg font rendering glitch. @tacaswell, is there anything else you want to do on this before it is merged? Maybe squash down to a single commit?

@efiring
Copy link
Member

efiring commented Dec 12, 2014

I think it needs an API_CHANGES entry.

This is needed because this test tries to add artists from one axes to
another which is not really supported.  Not 100% that this is fully
equivalent to the current test.  There is a call to `add_collection` in
scatter, but the test might be defeated by some of the other logic in
scatter.
Trying to set the axes property before calling up the mro to the base
class (eventually Artist) `__init__` which means that the `_axes`
attribute is not yet attached to the instance object.

 - removed setting of `self.axes`, this is taken care of in the
   baseclass
 - moved rest of sub-class specific attributes to after call up mro
   stack
The actual processing and artist creation in Axes.plot happens through
the `__call__` method on a _process_plot_var_args instance hanging off
the Axes object.  The _process_plot_var_args knows what axes it is
hanging off of and passed that into the Line2D artists as a kwarg at
creation time, which sets the axes of the Line2D.  The axes of the Line2D
is set again during the `Axes.add_line` method.  For the most part this
is fine, if un-needed, because the axes is the same objects both time
it is set.  However when using the mpl_toolkit, Axes objects get wrapped
in not-strictly-subclassed objects and the Axes object that the
_process_plot_var_args object knows about is not the Axes object that
add_line is called on which now results in an exception as it is moving
artists between axes.
 - deprecate Artist.{get,set}_axes
 - add axes property
 - Raise exception when trying to move artists between axes.
@tacaswell
Copy link
Member Author

On a silly vanity thing, before I squashed/rebased I had a commit on Nov 28 which would have filled in a gap in what gh thought my longest streak was.

I want to keep the three first commits as separate commits because they are each atomic non-backwards (I hope!) changes ( 69dece should win an award for ratio of commit message length to lines changed).

Re-ordered into an idealized version of history.

efiring added a commit that referenced this pull request Dec 13, 2014
@anntzer
Copy link
Contributor

anntzer commented Jul 18, 2016

<sorry for necro'ing this thread>
This patch does not prevent changing the axes of an artist by setting it first to None, then to a new axes. Similarly, nothing is done regarding figure reassignment.
While this may look like somewhat of an edge case, this can lead to all kinds of weirdness wrt statful callbacks such as #6785 (I don't want to imagine what would happen if someone reassigned the axes or figure of a draggable artist in some callback). Thus, I'd suggest the following changes:

  • the .axes and .figure attributes can only ever be set to a non-None value once (they can be reset multiple times to the same value).
  • the .figure attribute is automatically set when .axes is set.

Thoughts?

@jenshnielsen
Copy link
Member

Sounds reasonable to me. Will you put that in an issue or a new pr? Comments on old closed PRs are very likely to be lost

@anntzer
Copy link
Contributor

anntzer commented Jul 18, 2016

I'll make a PR.
Edit: actually, just an issue for now.

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

Successfully merging this pull request may close these issues.

6 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.