Skip to content

Allow sharing Locators and Formatters across Axises. #13482

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

Closed
wants to merge 1 commit into from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 21, 2019

PR Summary

Perhaps a better solution than #13439 to #13438.

Needs tests.

There's still quite a bit of code, but it's much less involved I think.

Edit: also closes #19988 as a side effect.

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

@timhoffm
Copy link
Member

So, the fundamental idea of this PR is to reassign the axis whenever we‘re obtaining the locator/formatter from the axis?

This feels quite hacky, because reading an axis property modifies the state of the returned object. I wouldn‘t expect axis.locator to modify any state. It‘s probably working for the common use cases. But one can certainly construct code with surprising effects based on this PR.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 23, 2019

So, the fundamental idea of this PR is to reassign the axis whenever we‘re obtaining the locator/formatter from the axis?

Yes.

This feels quite hacky, because reading an axis property modifies the state of the returned object. I wouldn‘t expect axis.locator to modify any state. It‘s probably working for the common use cases. But one can certainly construct code with surprising effects based on this PR.

If the locator/formatter is used on only one axis, this doesn't change anything. If it is used on multiple axises simultaneously, this at least fixes certain cases that were previously not working.

@ImportanceOfBeingErnest
Copy link
Member

Possibly relevant for further discussion: Here is a real user struggling with reusage of tickers.

@ImportanceOfBeingErnest ImportanceOfBeingErnest added this to the v3.3.0 milestone Aug 29, 2019
@anntzer anntzer force-pushed the shared-locator-formatter branch 2 times, most recently from 9ff4d61 to 1a23fd3 Compare August 30, 2019 16:42
@anntzer
Copy link
Contributor Author

anntzer commented Aug 30, 2019

A side effect of rebasing showed that the isDefault_{maj,min}{loc,fmt} attributes should also really live on the ticker instances; moving them there (using properties to redirect access, yada yada) gave a nice simplification over #14997.

@efiring
Copy link
Member

efiring commented May 28, 2020

If we really want to "fix" this, I think a better solution may be to make ticker not store the axis info and make get_major_formatter() (and friends) create a temporary FormatterWithAxisAttached object that wraps the Formatter and attaches the Axis info to it.

The proposal by @anntzer at the end of #13439 (comment) (above) seems like the cleanest solution, given the way we are boxed in by back-compatibility constraints. Conceptually, it seems like the fundamental problem is that the machinery--the algorithm and initialized parameters of a given ticker--should be fully shareable and re-usable, only needing to be created once by the user if it is needed multiple times. Info from the axis should be supplied when needed for the machinery to run--when generating specific ticks or strings. Ideally, perhaps the tickers would have been designed so that each method takes the axis, or the info required from the axis, as an explicit argument instead of referring to a stored axis attribute.

If I understand correctly, the present proposal is supplying that axis info by modifying a possibly shared instance each time it is accessed for use; I agree with @timhoffm that this doesn't feel quite right, though I also see its advantage over the present situation.

I think the proposal in the cited comment would get us closer to what we actually want, but I don't know what obstacles it would run into.

@anntzer
Copy link
Contributor Author

anntzer commented May 28, 2020

IIRC (it's been a while, didn't have a look again, just typing this from memory) the problem with FormatterWithAxisAttached is that it's even more work to implement and you need to make sure that it quacks enough like a normal Formatter, and then if someone calls get_major_formatter() and modifies that instance in-place (via one one the numerous setters that exist) they certainly expect the real formatter for be modified so you need to send the updates back to the underlying real Formatter. Well, you can always give it a try :-)

@jklymak
Copy link
Member

jklymak commented Nov 25, 2020

Can we make an issue that shows what breaks due to the sharing? I'm not sure this approach is the easiest one, but maybe there are use cases I a not understanding.

Naively I'd just make a copy of the locator or formatter if self.axis is not None when we call set_major_formatter or set_major_locator etc...

@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 22, 2021
@anntzer anntzer force-pushed the shared-locator-formatter branch from f4e4f26 to ec75fbb Compare April 16, 2021 16:39
@anntzer
Copy link
Contributor Author

anntzer commented Apr 16, 2021

@jklymak I think the issues are described in #13438 (comment) and the subsequent discussion?

@jklymak
Copy link
Member

jklymak commented Apr 27, 2021

I'm marking draft for now until we resolve the design here. Maybe better done in #13438....

@QuLogic QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 23, 2021
@anntzer anntzer force-pushed the shared-locator-formatter branch from ec75fbb to a142ed0 Compare November 2, 2021 07:17
@QuLogic QuLogic modified the milestones: v3.6.0, v3.7.0 Jul 5, 2022
@tacaswell tacaswell modified the milestones: v3.7.0, v3.8.0 Dec 15, 2022
@github-actions
Copy link

github-actions bot commented Jun 1, 2023

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Jun 1, 2023
@anntzer
Copy link
Contributor Author

anntzer commented Jun 1, 2023

Ditto #13439 (comment).

@anntzer anntzer closed this Jun 1, 2023
@anntzer anntzer deleted the shared-locator-formatter branch June 1, 2023 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removal of x-shared polar axes causes crash
7 participants