BUG: use more generic type inference for fast plotting #15094

jreback · 2017-01-10T00:42:26Z

https://travis-ci.org/pandas-dev/pandas/jobs/190356982

was failing on tz-aware dtypes. these routines are the same in that they require datetime64ns (but any also accepts tz-aware).

jreback · 2017-01-10T00:42:35Z

@jorisvandenbossche

codecov-io · 2017-01-10T04:21:41Z

Current coverage is 84.75% (diff: 90.90%)

Merging #15094 into master will increase coverage by <.01%

@@             master     #15094   diff @@
==========================================
  Files           145        145          
  Lines         51232      51234     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43420      43423     +3   
+ Misses         7812       7811     -1   
  Partials          0          0

Powered by Codecov. Last update 0fe491d...c64b56b

jorisvandenbossche

Good catch!
Forgot to look at the 'allowed failures' ..

jorisvandenbossche · 2017-01-10T20:06:32Z

pandas/tseries/converter.py

@@ -172,7 +172,7 @@ def _dt_to_float_ordinal(dt):
    is a :func:`float`.
    """
    if (isinstance(dt, (np.ndarray, Index, Series)
-                   ) and is_datetime64_ns_dtype(dt)):


Theoretically, this is not fully correct, as is_datetime64_any_dtype can also accept datetime64 values with other than ns resolution?
Although I don't think you can end up here with such values when going through the DatetimeConverter, as datetime64 arrays first get passed through to_datetime

We could maybe add a test for this (and for tz aware Timestamp): https://github.com/pandas-dev/pandas/blob/master/pandas/tseries/tests/test_converter.py#L58

Actually, we already have this:

rs = self.dtc.convert(np.array([ np_datetime64_compat('2012-01-01 00:00:00+0000'), np_datetime64_compat('2012-01-02 00:00:00+0000')]), None, None) self.assertEqual(rs[0], xp)

does already test this as the above has [s] resolution, not [ns] .

Would just add a test case for the tz case.

jreback · 2017-01-10T21:34:18Z

ok, I fixed is_datetime64_ns_dtype to work with tz-aware (as any does as well, this makes sense). let's see if anything else breaks. added a test as well.

jreback · 2017-01-11T00:01:21Z

ok, this is actually a tiny public API change, but it was inconsistent / wrong before.

jreback · 2017-01-11T19:00:13Z

@jorisvandenbossche

jorisvandenbossche · 2017-01-11T20:24:54Z

How safe is it to change this? Couldn't there be places in our code where is_datetime64_ns_dtype is that assumes it has no timezone?

Just as overview, is this correct?

is_datetime64_dtype -> any resolution, no tz (pure numpy)
is_datetime64_any_dtype -> any resolution, with or without tz
is_datetime64tz_dtype -> ns? resolution, with tz (as other resolution with tz is not possible I think?)
is_datetime64_ns_dtype -> ns resolution, with or without tz

The combination missing is "ns resolution, no tz (pure numpy)", which is actually what is_datetime64_ns_dtype was before. But maybe we don't need this? (as you can use the tz one to check it has no timezone)

jorisvandenbossche · 2017-01-11T20:15:27Z

pandas/tseries/tests/test_converter.py

+        self.assertEqual(rs[1], xp)
+
+        rs = self.dtc.convert(ts, None, None)
+        self.assertEqual(rs, xp)


This seems duplicate of line 71-72 ?

Can you also add similar test for Index([ts - Day(1), ts]).to_pydatetime to test tz aware datetime.datetime objects (as this is actually what was raising the tests I think, as _mpl_repr() does return that).

jorisvandenbossche · 2017-01-11T20:21:57Z

pandas/tests/types/test_inference.py

+        self.assertTrue(is_datetime64_any_dtype('datetime64[ns]'))
+        self.assertTrue(is_datetime64_any_dtype(ts))
+        self.assertTrue(is_datetime64_any_dtype(tsa))
+


Can you do the exact same 4 tests for is_datetime64tz_dtype?

jreback · 2017-01-11T20:33:03Z

these functions are a bit redundant; the _ns_ and _any_ are barely used at all. so I don't think this is a big deal to change.

jorisvandenbossche · 2017-01-11T20:37:56Z

Indeed, my search says that _any_ is exactly used in one spot, and _ns_ in three :-). So maybe rather something to clean-up? (but not for this PR)

jreback · 2017-01-11T20:41:35Z

@jorisvandenbossche yeah I'd leave them for now, they are useful, even if not used :>

pushing in a sec

xref pandas-dev#15073

jreback · 2017-01-12T23:14:25Z

@jorisvandenbossche

jorisvandenbossche · 2017-01-12T23:33:34Z

yes!

) xref pandas-dev#15073

jreback added Dtype Conversions Unexpected or buggy dtype conversions Visualization plotting labels Jan 10, 2017

jreback added this to the 0.20.0 milestone Jan 10, 2017

jorisvandenbossche reviewed Jan 10, 2017

View reviewed changes

jreback force-pushed the fix_plot branch from 4a0b368 to cf5a666 Compare January 10, 2017 21:33

jreback force-pushed the fix_plot branch from cf5a666 to 275f1ed Compare January 11, 2017 00:00

jreback force-pushed the fix_plot branch from 275f1ed to 4df1821 Compare January 11, 2017 02:17

jorisvandenbossche reviewed Jan 11, 2017

View reviewed changes

BUG: use more generic type inference for fast plotting

c64b56b

xref pandas-dev#15073

jreback force-pushed the fix_plot branch from 4df1821 to c64b56b Compare January 11, 2017 20:42

jorisvandenbossche merged commit c9e7aee into pandas-dev:master Jan 12, 2017

AnkurDedania pushed a commit to AnkurDedania/pandas that referenced this pull request Mar 21, 2017

BUG: use more generic type inference for fast plotting (pandas-dev#15094

519d488

) xref pandas-dev#15073

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: use more generic type inference for fast plotting #15094

BUG: use more generic type inference for fast plotting #15094

jreback commented Jan 10, 2017

jreback commented Jan 10, 2017

codecov-io commented Jan 10, 2017 •

edited

Loading

jorisvandenbossche left a comment

jorisvandenbossche Jan 10, 2017

jorisvandenbossche Jan 10, 2017

jorisvandenbossche Jan 10, 2017

jreback commented Jan 10, 2017

jreback commented Jan 11, 2017

jreback commented Jan 11, 2017

jorisvandenbossche commented Jan 11, 2017

jorisvandenbossche Jan 11, 2017

jorisvandenbossche Jan 11, 2017

jorisvandenbossche Jan 11, 2017

jreback commented Jan 11, 2017

jorisvandenbossche commented Jan 11, 2017

jreback commented Jan 11, 2017

jreback commented Jan 12, 2017

jorisvandenbossche commented Jan 12, 2017

BUG: use more generic type inference for fast plotting #15094

BUG: use more generic type inference for fast plotting #15094

Conversation

jreback commented Jan 10, 2017

jreback commented Jan 10, 2017

codecov-io commented Jan 10, 2017 • edited Loading

Current coverage is 84.75% (diff: 90.90%)

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jorisvandenbossche Jan 10, 2017

Choose a reason for hiding this comment

jorisvandenbossche Jan 10, 2017

Choose a reason for hiding this comment

jorisvandenbossche Jan 10, 2017

Choose a reason for hiding this comment

jreback commented Jan 10, 2017

jreback commented Jan 11, 2017

jreback commented Jan 11, 2017

jorisvandenbossche commented Jan 11, 2017

jorisvandenbossche Jan 11, 2017

Choose a reason for hiding this comment

jorisvandenbossche Jan 11, 2017

Choose a reason for hiding this comment

jorisvandenbossche Jan 11, 2017

Choose a reason for hiding this comment

jreback commented Jan 11, 2017

jorisvandenbossche commented Jan 11, 2017

jreback commented Jan 11, 2017

jreback commented Jan 12, 2017

jorisvandenbossche commented Jan 12, 2017

codecov-io commented Jan 10, 2017 •

edited

Loading