remove unused time conversion funcs #17711

jbrockmendel · 2017-09-28T21:41:04Z

Takes the place of #17708, removing funcs instead of moving them.

Had to cpdef pydt_to_i8 because the func it is replacing in _libs.index was cdef'd.

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

jreback · 2017-09-28T23:38:49Z

pandas/_libs/index.pyx

@@ -415,12 +406,12 @@ cdef class DatetimeEngine(Int64Engine):
            if not self.is_unique:
                return self._get_loc_duplicates(val)
            values = self._get_index_values()
-            conv = _to_i8(val)


isn't this a cimport?

At the moment it is cdefd in _libs.index. This replaces it with a call to tslib.pydt_to_i8. As noted in the OP, pydt_to_i8 is currently just def, but in the PR is made cpdef so that the c version can be called here (tslib is cimported into _libs.index)

jbrockmendel · 2017-09-29T04:29:21Z

Haven't gotten a handle on why, but using pydt_to_i8 in place of _to_i8 in _libs.index causes a handful of test failures. The main difference AFAICT is that _to_i8 is less reliable, returns some types of arguments unchanged.

So for the time being, this moves _to_i8 to tslib which is at least a more reasonable place for it to hang out. The other removals discussed are still removals.

jreback · 2017-09-29T10:12:55Z

pandas/_libs/tslib.pyx

@@ -3436,7 +3436,24 @@ def cast_to_nanoseconds(ndarray arr):
    return result


-def pydt_to_i8(object pydt):
+cdef inline _to_i8(object val):


you need a return type (int64_t)

There are cases in which it returns the object input.

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

pandas/_libs/tslib.pyx

+    try:
+        return val.value
+    except AttributeError:
+        if is_datetime64_object(val):


just Timestamp(val).value is prob enough here, this looks like really old conversion code

did u fix for this?

See #17711 (comment)

so just change this to (to remove all of this code)

try return val.value except AttributeError: if not is_string_object(val): return Timestamp(val).value return val

of maybe is isinstance(val, (np.datetime64, datetime))

Implemented this and it doesn't appear to have broken anything.

great, ok merging this. would still like to remove this routing (in favor of pydt_to_i8 but can do that in another PR.

codecov · 2017-09-29T16:38:43Z

Codecov Report

Merging #17711 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #17711      +/-   ##
==========================================
- Coverage   91.27%   91.23%   -0.05%     
==========================================
  Files         163      163              
  Lines       49766    49783      +17     
==========================================
- Hits        45423    45418       -5     
- Misses       4343     4365      +22

Flag	Coverage Δ
#multiple	`89.02% <0%> (-0.03%)`	⬇️
#single	`40.34% <0%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/pytables.py	`92.79% <0%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/plotting/_converter.py	`63.38% <0%> (-1.82%)`	⬇️
pandas/core/frame.py	`97.73% <0%> (-0.1%)`	⬇️
pandas/core/sparse/array.py	`91.58% <0%> (ø)`	⬆️
pandas/compat/numpy/function.py	`93.33% <0%> (+0.2%)`	⬆️

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 cc58b84...84030ed. Read the comment docs.

codecov · 2017-09-29T16:38:55Z

Codecov Report

Merging #17711 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #17711      +/-   ##
==========================================
- Coverage   91.27%   91.24%   -0.04%     
==========================================
  Files         163      163              
  Lines       49766    49779      +13     
==========================================
- Hits        45423    45419       -4     
- Misses       4343     4360      +17

Flag	Coverage Δ
#multiple	`89.03% <0%> (-0.02%)`	⬇️
#single	`40.32% <0%> (-0.08%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/pytables.py	`92.79% <0%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
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/resample.py	`96.05% <0%> (-0.12%)`	⬇️
pandas/core/frame.py	`97.73% <0%> (-0.11%)`	⬇️
pandas/core/indexes/interval.py	`92.85% <0%> (ø)`	⬆️
pandas/core/sparse/array.py	`91.58% <0%> (ø)`	⬆️
pandas/core/groupby.py	`92.25% <0%> (+0.01%)`	⬆️
... and 3 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 cc58b84...95f778a. Read the comment docs.

* '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) ...

remove unused time conversion funcs

0501340

jreback reviewed Sep 28, 2017

View reviewed changes

revert to using _to_i8

c78e766

jreback requested changes Sep 29, 2017

View reviewed changes

jreback added Clean Datetime Datetime data dtype labels Sep 29, 2017

remove unused imports

84030ed

use Timestamp per reviewier request

95f778a

jreback added this to the 0.21.0 milestone Oct 1, 2017

jreback approved these changes Oct 1, 2017

View reviewed changes

jreback merged commit dead59a into pandas-dev:master Oct 1, 2017

jbrockmendel deleted the tslibs-conversion2 branch October 30, 2017 16:23

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

remove unused time conversion funcs (pandas-dev#17711)

3f668b5

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

remove unused time conversion funcs (pandas-dev#17711)

f80ae51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove unused time conversion funcs #17711

remove unused time conversion funcs #17711

jbrockmendel commented Sep 28, 2017

jreback Sep 28, 2017

jbrockmendel Sep 28, 2017

jbrockmendel commented Sep 29, 2017

jreback Sep 29, 2017

jbrockmendel Sep 29, 2017

jreback Sep 29, 2017

jreback Sep 30, 2017

jbrockmendel Sep 30, 2017

jreback Oct 1, 2017 •

edited

Loading

jbrockmendel Oct 1, 2017

jreback Oct 1, 2017

codecov bot commented Sep 29, 2017

codecov bot commented Sep 29, 2017 •

edited

Loading

remove unused time conversion funcs #17711

remove unused time conversion funcs #17711

Conversation

jbrockmendel commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Sep 29, 2017

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 Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 29, 2017

Codecov Report

codecov bot commented Sep 29, 2017 • edited Loading

Codecov Report

jreback Oct 1, 2017 •

edited

Loading

codecov bot commented Sep 29, 2017 •

edited

Loading