Separate out _convert_datetime_to_tsobject #17715

jbrockmendel · 2017-09-29T02:49:22Z

A bunch of calls to convert_to_tsobject are made in cases where we already know that the input is a datetime object. This PR separates out the datetime case into a new function _convert_datetime_to_tsobject.

Second, to make the dependency between _TSObject and Timestamp one-way, this removes the three references to Timestamp that are made in convert_str_to_tsobject. Verifying the last of these changes may be non-trivial.

This is a prelude to separating _TSobject logic out into tslibs.conversion.

closes #xxxx
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

codecov · 2017-09-29T05:19:24Z

Codecov Report

Merging #17715 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #17715      +/-   ##
==========================================
- Coverage   91.27%   91.25%   -0.03%     
==========================================
  Files         163      163              
  Lines       49783    49783              
==========================================
- Hits        45439    45429      -10     
- Misses       4344     4354      +10

Flag	Coverage Δ
#multiple	`89.05% <ø> (-0.01%)`	⬇️
#single	`40.34% <ø> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.73% <0%> (-0.1%)`	⬇️
pandas/core/indexes/datetimes.py	`95.44% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbf0dda...6ced748. Read the comment docs.

codecov · 2017-09-29T05:19:57Z

Codecov Report

Merging #17715 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #17715      +/-   ##
==========================================
- Coverage   91.27%   91.21%   -0.06%     
==========================================
  Files         163      163              
  Lines       49783    49796      +13     
==========================================
- Hits        45439    45421      -18     
- Misses       4344     4375      +31

Flag	Coverage Δ
#multiple	`89.01% <ø> (-0.05%)`	⬇️
#single	`40.33% <ø> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/plotting/_converter.py	`63.38% <0%> (-1.82%)`	⬇️
pandas/core/tools/datetimes.py	`82.97% <0%> (-0.83%)`	⬇️
pandas/core/indexes/multi.py	`96.39% <0%> (-0.51%)`	⬇️
pandas/core/indexes/category.py	`97.46% <0%> (-0.29%)`	⬇️
pandas/core/groupby.py	`92.04% <0%> (-0.2%)`	⬇️
pandas/core/frame.py	`97.73% <0%> (-0.11%)`	⬇️
pandas/core/resample.py	`96.14% <0%> (-0.03%)`	⬇️
pandas/core/indexes/interval.py	`92.85% <0%> (ø)`	⬆️
pandas/core/dtypes/inference.py	`98.36% <0%> (+0.02%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbf0dda...9deb75b. Read the comment docs.

jreback · 2017-09-29T09:58:55Z

pandas/_libs/tslib.pyx

@@ -728,7 +728,7 @@ class Timestamp(_Timestamp):
        # reconstruct & check bounds
        ts_input = datetime(dts.year, dts.month, dts.day, dts.hour, dts.min,
                            dts.sec, dts.us, tzinfo=_tzinfo)
-        ts = convert_to_tsobject(ts_input, _tzinfo, None, 0, 0)
+        ts = _convert_datetime_to_tsobject(ts_input, _tzinfo)


no need to break with spelling convention. call convert_from_datetime_to_tsobject

jreback · 2017-09-29T10:00:59Z

pandas/_libs/tslib.pyx

+                                     obj2.dts.day,
+                                     obj2.dts.hour, obj2.dts.min, obj2.dts.sec,
+                                     obj2.dts.us, obj2.tzinfo)
+                    obj2 = _convert_datetime_to_tsobject(dtime, tz)


have convert_datetime_to_tsobject take an optional ps= to facilitate this type of post-conversion.

also then you don't need a obj2 avoiding more confusion here.

jreback · 2017-09-29T10:02:49Z

pandas/_libs/tslib.pyx

@@ -1577,7 +1589,17 @@ cpdef convert_str_to_tsobject(object ts, object tz, object unit,
                    return obj
                else:
                    # Keep the converter same as PyDateTime's
-                    ts = Timestamp(obj.value, tz=obj.tzinfo)
+                    obj2 = convert_to_tsobject(obj.value, obj.tzinfo,


this is just a variation on create_datatime_from_ts

I don't follow.

jreback · 2017-09-29T10:03:22Z

pandas/_libs/tslib.pyx

@@ -1538,12 +1497,64 @@ cdef convert_to_tsobject(object ts, object tz, object unit,
    return obj


+cdef _TSObject _convert_datetime_to_tsobject(datetime ts, object tz):
+    cdef:


pls pls always add doc-strings

jreback

does perf change? (pos or neg)?

jreback · 2017-09-30T21:33:01Z

pandas/_libs/tslib.pyx

+                                            int32_t nanos=0):
+    """
+    Extract datetime and int64 from any of:
+        - python datetime object


can u put a proper doc string

This is adapted from the convert_to_tsobject docstring.

take the opportunity to improve it

jreback · 2017-09-30T21:34:11Z

pandas/_libs/tslib.pyx

-                    ts = Timestamp(obj.value, tz=obj.tzinfo)
+                    obj = convert_to_tsobject(obj.value, obj.tzinfo,
+                                              None, 0, 0)
+                    dtime = datetime(obj.dts.year, obj.dts.month, obj.dts.day,


use the existing routine to do this

Are you referring to create_datetime_from_ts?

create_datetime_from_ts is a compatibility func specifically for ints_to_pydatetime (you can tell because it has several unused arguments that are retained to match the signature of create_timestamp_from_ts).

Replacing a one-line call to datetime(...) with a call to a one-line func that does exactly the same thing (but with less clarity because of the unused args) doesn't achieve much, with the possible exception of function call overhead.

I know I wrote it!

yet you are repeating code, I would prefer not to.

with the possible exception of function call overhead.

these take / return objects so the overhead is the same.

jbrockmendel · 2017-10-01T01:41:14Z

No significant changes

$asv continuous -f 1.1 -E virtualenv master HEAD -b timeseries
[...]
       before           after         ratio
     [cc58b84f]       [a7b66c1a]
+        11.8±0ms         15.9±2ms     1.35  timeseries.DatetimeIndex.time_infer_freq_none
+           1.55s            2.05s     1.32  timeseries.AsOfDataFrame.time_asof_nan
+     19.6±0.07μs         24.0±3μs     1.23  timeseries.Offsets.time_timeseries_day_apply
+      16.6±0.1μs       18.3±0.1μs     1.10  timeseries.Offsets.time_custom_bday_apply
-       174±0.9μs        158±0.3μs     0.91  timeseries.Offsets.time_custom_bmonthend_decr_n
-        422±20ms        380±0.2ms     0.90  timeseries.DatetimeIndex.time_add_offset_slow
-     22.1±0.09μs      19.8±0.05μs     0.90  timeseries.Offsets.time_timeseries_day_incr
-           1.99s        1.48±0.3s     0.74  timeseries.AsOfDataFrame.time_asof

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.

jreback · 2017-10-01T18:13:14Z

pandas/_libs/tslib.pyx

@@ -1577,7 +1614,14 @@ cpdef convert_str_to_tsobject(object ts, object tz, object unit,
                    return obj
                else:
                    # Keep the converter same as PyDateTime's
-                    ts = Timestamp(obj.value, tz=obj.tzinfo)
+                    obj = convert_to_tsobject(obj.value, obj.tzinfo,


ok so this is getting cumbersome. why don't you make a separate routine to do these 3 (and I guess go back to consrructing datetime). But why does the

ts = Timestamp(obj.value, tz=obj.tzinfo) not work here?

why don't you make a separate routine to do these 3 (and I guess go back to consrructing datetime)

OK

But why does the ts = Timestamp(obj.value, tz=obj.tzinfo) not work here?

It works, but the goal is to disentangle Timestamp from _TSObject logic. Or more specifically, to make the dependency go in one direction.

jreback · 2017-10-01T20:03:08Z

ok ping on green. Still would like to simplify some of this logic generally (as the is pr just moves it around), but can do a bit later (maybe add to TODO).

jbrockmendel · 2017-10-01T20:10:58Z

Still would like to simplify some of this logic generally

Yah. As I look at it, some pieces might be simpler/clearer as methods (or classmethods) of _TSObject). Would that defeat the purpose of _TSObject, since it is explicitly supposed to be "lightweight"?

jbrockmendel · 2017-10-01T23:22:10Z

Not sure what's up here. Tests started failing when create_datetime_from_ts was introduced, continue failing with just _convert_tsobject_tz. My inclination is to revert to two commits ago when it was passing and add this mystery to the TODO list.

jbrockmendel · 2017-10-02T03:43:47Z

ping, though note reversion mentioned above.

jreback · 2017-10-02T12:43:43Z

thanks

* 'master' of github.com:pandas-dev/pandas: (188 commits) Separate out _convert_datetime_to_tsobject (pandas-dev#17715) DOC: remove whatsnew note for xref pandas-dev#17131 BUG: Regression in .loc accepting a boolean Index as an indexer (pandas-dev#17738) DEPR: Deprecate cdate_range and merge into bdate_range (pandas-dev#17691) CLN: replace %s syntax with .format in pandas.core: categorical, common, config, config_init (pandas-dev#17735) Fixed the memory usage explanation of categorical in gotchas from O(nm) to O(n+m) (pandas-dev#17736) TST: add backward compat for offset testing for pickles (pandas-dev#17733) remove unused time conversion funcs (pandas-dev#17711) DEPR: Deprecate convert parameter in take (pandas-dev#17352) BUG:Time Grouper bug fix when applied for list groupers (pandas-dev#17587) BUG: Fix some PeriodIndex resampling issues (pandas-dev#16153) BUG: Fix unexpected sort in groupby (pandas-dev#17621) DOC: Fixed typo in documentation for 'pandas.DataFrame.replace' (pandas-dev#17731) BUG: Fix series rename called with str altering name rather index (GH17407) (pandas-dev#17654) DOC: Add examples for MultiIndex.get_locs + cleanups (pandas-dev#17675) Doc improvements for IntervalIndex and Interval (pandas-dev#17714) BUG: DataFrame sort_values and multiple "by" columns fails to order NaT correctly Last of the timezones funcs (pandas-dev#17669) Add missing file to _pyxfiles, delete commented-out (pandas-dev#17712) update imports of DateParseError, remove unused imports from tslib (pandas-dev#17713) ...

jbrockmendel added 2 commits September 28, 2017 19:19

Separate out _convert_datetime_to_tsobject

a9f2778

typo fixup

6ced748

jreback requested changes Sep 29, 2017

View reviewed changes

Simplifications per reviewer suggestions

a7b66c1

sinhrks added the Clean label Sep 29, 2017

jreback requested changes Sep 30, 2017

View reviewed changes

jbrockmendel added 2 commits September 30, 2017 18:44

verbosify docstring per reviewer request

4392577

at reviewer request, use inconvenient convenience routine

21ec866

jreback reviewed Oct 1, 2017

View reviewed changes

Separate out _convert_tsobject_tz

509699e

jreback added this to the 0.21.0 milestone Oct 1, 2017

jreback approved these changes Oct 1, 2017

View reviewed changes

jreback removed this from the 0.21.0 milestone Oct 1, 2017

revert _convert_tsobject_tz

9deb75b

jreback added this to the 0.21.0 milestone Oct 2, 2017

jreback merged commit a3d538a into pandas-dev:master Oct 2, 2017

jbrockmendel deleted the tslibs-pre_conversion branch October 30, 2017 16:25

alanbato pushed a commit to alanbato/pandas that referenced this pull request Nov 10, 2017

Separate out _convert_datetime_to_tsobject (pandas-dev#17715)

fc13ce3

No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

Separate out _convert_datetime_to_tsobject (pandas-dev#17715)

d96fd37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate out _convert_datetime_to_tsobject #17715

Separate out _convert_datetime_to_tsobject #17715

jbrockmendel commented Sep 29, 2017

codecov bot commented Sep 29, 2017 •

edited

Loading

codecov bot commented Sep 29, 2017 •

edited

Loading

jreback Sep 29, 2017

jreback Sep 29, 2017

jbrockmendel Sep 29, 2017

jreback Sep 29, 2017

jbrockmendel Sep 29, 2017

jreback Sep 29, 2017

jreback left a comment

jreback Sep 30, 2017

jbrockmendel Sep 30, 2017

jreback Sep 30, 2017

jreback Sep 30, 2017

jbrockmendel Oct 1, 2017

jreback Oct 1, 2017

jbrockmendel Oct 1, 2017

jreback Oct 1, 2017

jbrockmendel Oct 1, 2017

jbrockmendel commented Oct 1, 2017

jreback Oct 1, 2017

jbrockmendel Oct 1, 2017

jreback commented Oct 1, 2017

jbrockmendel commented Oct 1, 2017

jbrockmendel commented Oct 1, 2017

jbrockmendel commented Oct 2, 2017

jreback commented Oct 2, 2017

Separate out _convert_datetime_to_tsobject #17715

Separate out _convert_datetime_to_tsobject #17715

Conversation

jbrockmendel commented Sep 29, 2017

codecov bot commented Sep 29, 2017 • edited Loading

Codecov Report

codecov bot commented Sep 29, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Oct 1, 2017

jbrockmendel commented Oct 1, 2017

jbrockmendel commented Oct 1, 2017

jbrockmendel commented Oct 2, 2017

jreback commented Oct 2, 2017

codecov bot commented Sep 29, 2017 •

edited

Loading

codecov bot commented Sep 29, 2017 •

edited

Loading