Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

BUG: invert_xaxis (negative tot_sec) triggers MilliSecondLocator (#3990) #3991

Merged
merged 1 commit into from Jul 10, 2013

Conversation

Projects
None yet
3 participants

Solves issue #3990.
Negative timedelta for xaxis range will no longer trigger MilliSecondLocator.

Do you think this is a correct fix?
And do I have to include some more things (test, mention in release notes, ..)

Contributor

jreback commented Jul 10, 2013

didn't see this....does this resolve the issue?

@cpcloud can you take a look?

Member

cpcloud commented Jul 10, 2013

there must be a reason that they are negative

@jorisvandenbossche can you explain why this fixes the problem? the relativedelta is allowed to be negative, but on the other hand, maybe whoever wrote this didn't take that into account.

Member

cpcloud commented Jul 10, 2013

e.g.

In [19]: a,b=date_range('20010101', periods=2)

In [20]: from dateutil.relativedelta import relativedelta

In [21]: d=relativedelta(a,b)

In [22]: delta = d

In [23]: paste
        num_days = ((delta.years * 12.0) + delta.months * 31.0) + delta.days
        num_sec = (delta.hours * 60.0 + delta.minutes) * 60.0 + delta.seconds
        tot_sec = num_days * 86400. + num_sec
## -- End pasted text --

In [24]: tot_sec
Out[24]: -86400.0

that doesn't like a case of "i forgot to abs", i could be wrong though.

For me this resolves the issue (but I submitted the patch of course :-) Although I don't really know if I could write a test for it.

relativedelta is indeed allowed to be negative, and is negative in the case of an inversed xaxis. That is not the problem. However, when deciding which tick Locator we want to use, it doesn't matter if it is inversed or not: we are interested in the absolute time delta. Because now the negative delta triggers a MilliSecondLocator just because it is negative, while the time delta is not suited for Milliseconds.

Some more details with references to code:
When using ax.invert_xaxis() the left and right xlim are interchanged. Because of this the delta in PandasAutoDateLocator will be negative (https://github.com/pydata/pandas/blob/master/pandas/tseries/converter.py#L241).
This in turn triggers MilliSecondLocator instead of the normal AutoDateLocator (because tot_sec in line https://github.com/pydata/pandas/blob/master/pandas/tseries/converter.py#L247 is negative), and causes the error as the datetime range is too big for MilliSecondLocator.

Member

cpcloud commented Jul 10, 2013

makes sense

@jreback i think this is ok.

Member

cpcloud commented Jul 10, 2013

@jorisvandenbossche thanks for the explanation!

Contributor

jreback commented Jul 10, 2013

ok..
@jorisvandenbossche can you add a quick release note?

OK, will do when I get home.

Thanks for the look at it!

I updated the commit with release notes. So for me it is ready to merge.

As I said in the issue, the only thing I don't understand is why the bug does not appear when using pandas plot method to create the axes object (so ax.plot(df.index, df['col'].values) vs df.plot(ax=ax))

Contributor

jreback commented Jul 10, 2013

ok....merging...thansk...keep us apprised if there are any addtl issues, or if you figure out the above (you can't open in issue if you want)

jreback added a commit that referenced this pull request Jul 10, 2013

Merge pull request #3991 from jorisvandenbossche/bug_invert_xaxis
BUG: invert_xaxis (negative tot_sec) triggers MilliSecondLocator (#3990)

@jreback jreback merged commit 098de41 into pandas-dev:master Jul 10, 2013

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