Movatterモバイル変換


[0]ホーム

URL:


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

Sign up
Appearance settings

Deprecate LocatableAxes from toolkits#10403

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
efiring merged 6 commits intomatplotlib:masterfromQuLogic:deprecate-locatable-axes
Jul 19, 2018

Conversation

QuLogic
Copy link
Member

@QuLogicQuLogic commentedFeb 9, 2018
edited
Loading

PR Summary

LocatableAxes in the toolkits provides stuff likeget_axes_locator, but these are provided in the baseAxes class. So deprecate all of the classes and related functions. Additionally, remove all code from the deprecatedaxes_grid, and point it to the split apart implementations.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • [N/A] New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • [N/A] 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

@QuLogicQuLogicforce-pushed thedeprecate-locatable-axes branch 3 times, most recently from9743d47 toe994a5fCompareFebruary 9, 2018 06:35
@tacaswelltacaswell added this to thev3.0 milestoneFeb 9, 2018
@tacaswell
Copy link
Member

I would rather push this to 3.0 (hoping to tag 2.2RC1 this weekend....only a week late)

@efiring
Copy link
Member

I haven't looked in detail, but I will be glad to see this go in whenever it does, and the duplication go away.

@QuLogicQuLogicforce-pushed thedeprecate-locatable-axes branch frome994a5f to2805e5cCompareFebruary 12, 2018 05:39
@QuLogic
Copy link
MemberAuthor

Updated deprecation messages to say 3.0 instead of 2.2.

@QuLogicQuLogicforce-pushed thedeprecate-locatable-axes branch from2805e5c to6dfa3ddCompareFebruary 22, 2018 21:23
@QuLogic
Copy link
MemberAuthor

Rebased due to conflicts. These files are messy, so it would be nice to get this in quicker to reduce conflicts.

Copy link
Member

@efiringefiring left a comment

Choose a reason for hiding this comment

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

This seems low-risk; let's get it in.

@efiring
Copy link
Member

@anntzer or@dopplershift, perhaps one of you could have a look and merge this if it looks OK, so it doesn't have to be rebased again.

@anntzer
Copy link
Contributor

I'm not even close to knowing enough about axes_locator to decide whether the functionality is indeed duplicated or not.

@QuLogic
Copy link
MemberAuthor

Mainly you can look at theLocatableAxesBase class which adds[gs]et_axes_locator and then modifiesapply_aspect to use it before drawing. But looking at the regular_AxesBase.draw, itdoes the same thing.

if 'sharex' in kwargs and 'sharey' in kwargs:
raise ValueError("Twinned Axes may share only one axis.")
ax2 = type(self)(self.figure, self.get_position(True), *kl, **kwargs)
ax2.set_axes_locator(self.get_axes_locator())
Copy link
Member

Choose a reason for hiding this comment

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

The baseAxes._make_twin_axes is missing this locator-copying line--maybe that is a bug.

@efiring
Copy link
Member

LocatableAxesBase dates all the way back to8e3b931, Dec. 9, 2008, when all of the AxesGrid family was started. That original commit (which looks like a combination of SVN commits?) includes the duplication ofget_axes_locator, etc., and its use indraw prior toapply_aspect, inAxes and inLocatableAxesBase. It looks to me like originally it was in the latter, and then@leejjoon decided to add it toAxes but didn't remove it fromLocatableAxesBase. The_axes_locator is used only in connection with the AxesGrid family, as far as I can see; there are references to it in tight_layout and bbox_inches=tight, but only the AxesGrid family actually sets it.

It looks to me like we need to either support_axes_locator more thoroughly inAxes (which at the moment means taking care to keep it synchronized in the twin case, requiring a modification of the setter as well as of_make_twin_axes), or strip it out ofAxes and leave the locatable axes class, factory function, etc. in place, possibly with some changes.

I'm not sure which would be best; it's a little odd to have an attribute inAxes that is used only by a toolkit, isn't it? On the other hand, the cost is fairly low.

@QuLogic
Copy link
MemberAuthor

There are times when@leejjoonrecommended usingaxes_locator and not the full toolkit, so I think the intention was to put it into the base library, but I can't say that 100%. I can also find arandom use in the wild, but it's hard to search when a bunch of people commit their venvs to git.

@jklymak
Copy link
Member

Ooh, this fell through the cracks. Any chance we can resurrect?

@jklymakjklymak modified the milestones:v3.0,v3.1Jul 9, 2018
@QuLogic
Copy link
MemberAuthor

I can rebase; depends on@efiring's thoughts.

@leejjoon
Copy link
Contributor

My opinion is highly biased as I am the one who introduced the axes_locator. But I would like to see it stays. While I understand this is underused, this makes certain things a lot easier (although may not relevant for normal users). Also,#11026 requires axes_locator.

@QuLogic
Copy link
MemberAuthor

This doesn't remove it though; it deprecates the one in axes_grid for the one in the main library, which appears to be what#11026 uses.

@leejjoon
Copy link
Contributor

Yes. I was just expressing my opinion to what@efiring said, and I was referring to axes_locator in the base. I am happy with the axes_grid becoming deprecated.

@efiring
Copy link
Member

I'm more than a little confused--I haven't looked at this for a while, it deals with functionality I am not familiar with, and so the spinup in trying to come back to it is considerable.@leejjoon, it's good to hear from you--but could you be more explicit with respect to this PR? Do you favor it as is (after a rebase), or with minor changes, or major changes, or not at all?

@leejjoon
Copy link
Contributor

I am in favor of this PR. In fact, I think we can actually removeaxes_grid toolkit from the code base.
On the other hand, I would prefer_axes_locator stays withinAxes. Let me know if I am still not clear.

@efiring
Copy link
Member

@leejjoon, thanks very much, that's what I thought.@QuLogic, let's try to get this in now. Once it is rebased and passes the tests, anyone is welcome to merge it.

@QuLogicQuLogicforce-pushed thedeprecate-locatable-axes branch from6dfa3dd to36e3701CompareJuly 18, 2018 22:54
This is provided for backwards-compatibility, so point directly to theimplementations instead of the compatibility location.
All its functionality is provided by the matplotlib.axes.Axes class nowso it does not need to exist as all alternative Axes classes derive fromthe main one.
It's similar to LocatableAxesBase and provided in base Axes classes.
It's now no longer used for anything, since maxes.Axes is locatablealready.
@QuLogicQuLogicforce-pushed thedeprecate-locatable-axes branch from36e3701 to7421b0eCompareJuly 18, 2018 23:04
@QuLogic
Copy link
MemberAuthor

OK, rebase is done.

@efiringefiring merged commit2eb26ee intomatplotlib:masterJul 19, 2018
@QuLogicQuLogic deleted the deprecate-locatable-axes branchJuly 19, 2018 08:06
@QuLogicQuLogic modified the milestones:v3.1,v3.0Jul 19, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@efiringefiringefiring approved these changes

@jklymakjklymakjklymak approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.0.0
Development

Successfully merging this pull request may close these issues.

6 participants
@QuLogic@tacaswell@efiring@anntzer@jklymak@leejjoon

[8]ページ先頭

©2009-2025 Movatter.jp