-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
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. |
Yes.
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. |
Possibly relevant for further discussion: Here is a real user struggling with reusage of tickers. |
9ff4d61
to
1a23fd3
Compare
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. |
1a23fd3
to
f4e4f26
Compare
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. |
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 :-) |
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 |
f4e4f26
to
ec75fbb
Compare
@jklymak I think the issues are described in #13438 (comment) and the subsequent discussion? |
I'm marking draft for now until we resolve the design here. Maybe better done in #13438.... |
ec75fbb
to
a142ed0
Compare
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. |
Ditto #13439 (comment). |
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