10000 Update AutoDateFormatter with locator by ImportanceOfBeingErnest · Pull Request #12470 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Update AutoDateFormatter with locator #12470

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 31, 2018
Merged

Update AutoDateFormatter with locator #12470

merged 3 commits into from
Oct 31, 2018

Conversation

ImportanceOfBeingErnest
Copy link
Member
@ImportanceOfBeingErnest ImportanceOfBeingErnest commented Oct 10, 2018 •
edited
Loading

PR Summary

When setting a date locator without setting a new formatter, the ticklabels become mostly useless.
This PR checks inside of the formatter call if the current locator is still the same as the one active on the axis and if not, sets its _locator to the new locator.

import matplotlib.pyplot as plt
import matplotlib.dates as mdates
from datetime import datetime

t = [datetime(2018,9,30,8,0), datetime(2018,9,30,8,59), datetime(2018,9,30,10,30)]
x = [2,3,1]

fig, ax = plt.subplots()
ax.plot(t,x)
ax.xaxis.set_major_locator(mdates.MinuteLocator((0,30)))
fig.autofmt_xdate()

Previously
scatterml_3 0 0

With this PR

scatterml_3 0 0rc1 post620 g74e066586

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

@ImportanceOfBeingErnest ImportanceOfBeingErnest force-pushed the updateformatterwithlocator branch 3 times, most recently from ddb55d2 to a35c323 Compare October 10, 2018 14:25
dstansby
dstansby reviewed Oct 10, 2018
View reviewed changes
lib/matplotlib/axis.py Outdated
"matplotlib.ticker.Locator")
self.isDefault_majloc = False
self.major.locator = locator
if (hasattr(self.get_major_formatter(), "_locator") and
hasattr(self.get_major_formatter(), "_tz")):
self.get_major_formatter()._locator = locator
Copy link
Member

Choose a reason for hiding this comment

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

Should this be .set_locator(locator) instead of ._locator?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Would you favour making locator a public property?

Copy link
Member
@dstansby dstansby Oct 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't really know, I was tripped up earlier in my stem PR by accessing something using a private attribute instead of its setter though!

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I don't think this should happen in axis.py at all, but should all happen in dates.py, probably in AutoDateLocator.__call__() where it can check if the locator on the self.axis has changed. I'll actually block on this, but am willing to be argued with if its really not possible any other way...

jklymak
jklymak previously requested changes Oct 10, 2018
View reviewed changes
lib/matplotlib/axis.py Outdated
"matplotlib.ticker.Locator")
self.isDefault_majloc = False
self.major.locator = locator
if (hasattr(self.get_major_formatter(), "_locator") and
hasattr(self.get_major_formatter(), "_tz")):
self.get_major_formatter()._locator = locator
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I don't think this should happen in axis.py at all, but should all happen in dates.py, probably in AutoDateLocator.__call__() where it can check if the locator on the self.axis has changed. I'll actually block on this, but am willing to be argued with if its really not possible any other way...

@ImportanceOfBeingErnest
Copy link
Member Author

Yep, makes sense. I'll see what's possible.

@ImportanceOfBeingErnest
Copy link
Member Author

@jklymak The problem is that Formatters in general do not know whether they are used as major or minor formatters. Hence, while the formatter can find out from self.axis which major and minor locators are used on the axis that it wants format, it cannot know which of the two it should use as locator.
To overcome this I now added a flag inside the AutoDateFormatter which will be set within set_minor_formatter.

@ImportanceOfBeingErnest ImportanceOfBeingErnest mentioned this pull request Oct 15, 2018
Merged
6 tasks
@jklymak
Copy link
Member
jklymak commented Oct 17, 2018

I'm not really following the minor locator hitch, and the test isn't particularly clear (at least to me). What use case breaks down if you don't keep track of the _ismajor flag? I don't mind dates.py being a bit of a mess, because the formatters need to know about the locators, but it'd be nice to avoid having that spill into tickers.py.

Long-term, I think there are plans to make locators and formatters less independent, so this is one of those cases where making that more general would be helpful.

@ImportanceOfBeingErnest
Copy link
Member Author
ImportanceOfBeingErnest commented Oct 17, 2018 •
edited
Loading

Neither a formatter, nor a locator have knowledge about which ticks they are being used with.
I.e. a locator does not know if it is used to position the major ticks or the minor ticks; and a formatter does not know if it is used to format the major or minor ticks.
The aim here is to keep the locator in sync with the formatter. While the formatter can find out which axis it belongs to, and hence can check if its ._locator is being used on that axis, it cannot find out if it is being used for minor or major ticks.
This is bypassed by setting the _ismajor attribute. With this being set when the formatter is registered as a major/minor formatter for an axis, the formatter now has knowledge about whether to check for the minor or major locator still being the same or not.

The usecase that would break down is probably this complete idea of being able to keep formatter and locator in sync automatically. The only alternative is see would be to have every formatter register itself to an axis, such that it could store whether it's minor or major formatter. That would surely affect the complete locator/formatter framework.

@jklymak
Copy link
Member
jklymak commented Oct 18, 2018

Hmm, OK< then I wonder if the right solution here is to add a set_locator and get_locator method to the base formatter class, and then call that from axis.set_major/minor_locator, like your original solution. Then, as you do here, the formatter will always know the locator value. I haven't thoight all the way through, but I think it can be made easily backwards compatible. Whether these methods should be private or not is a different question; if private maybe just make the base class have a ._locator

@ImportanceOfBeingErnest
Copy link
Member Author

@jklymak Ok, so here is a third attempt now. I went for a method _set_locator() on the Formatter, because this way it can just ignore any locator passed in (and not carry that instance with it for the rest of its life).

@jklymak
Copy link
Member
jklymak commented Oct 18, 2018

Yeah, but I think you want a getter as well, so that you dont' have to rely on AttributeErrors to know if the locator has been set or not (i.e. the default is to return None)

@ImportanceOfBeingErnest
Copy link
Member Author
ImportanceOfBeingErnest commented Oct 18, 2018 •
edited
Loading

Not sure I understand that. There will always be a locator being set, because you cannot instantiate the AutoDateFormatter without the positional argument.
The AttributeError is for the case you used a locator that doesn't have a _get_units.

Of course this isn't foolproof, but it has never been. E.g. you can still create AutoDateFormatter("my cool string") and it will eventually fail.

@jklymak
Copy link
Member
jklymak commented Oct 18, 2018

Sure, but in general it would be nice to have a way to get the locator associated w/ a formatter.

@ImportanceOfBeingErnest
Copy link
Member Author

Somehow I'm not happy about the idea of 90% of all formatters having a public method set_locator/get_locator that does nothing/returns None. We could introduce those for the AutoDateFormatter only, is that what you mean?

@jklymak
Copy link
Member
jklymak commented Oct 19, 2018

Sorry, I thought locator was a general argument for formatters, but its not. I think it should be. Some formatters take "loc", which is the same thing as taking the locator, but less flexible. I think they should be linked, but maybe implimenting that is too heavy a lift for this PR. The proposal here is a good stopgap, and all private so we can change later...

@dstansby dstansby merged commit 9e8be5b into matplotlib:master Oct 31, 2018
@dstansby dstansby added this to the v3.1 milestone Oct 31, 2018
@ImportanceOfBeingErnest ImportanceOfBeingErnest deleted the updateformatterwithlocator branch February 17, 2019 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
topic: date handling topic: ticks axis labels
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ImportanceOfBeingErnest @jklymak @dstansby
0