Move normalization funcs up to conversion #18086

jbrockmendel · 2017-11-03T03:57:15Z

We're going to need date_normalize upstream of tslib before long, so this moves it to conversion.

Simplifies repeated checking in localize_tso, closes #17944

gfyoung · 2017-11-03T06:23:17Z

Minor flake8 error. Otherwise, seems reasonable to me.

codecov · 2017-11-03T14:58:06Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #18086      +/-   ##
==========================================
- Coverage   91.27%   91.26%   -0.02%     
==========================================
  Files         163      163              
  Lines       50120    50120              
==========================================
- Hits        45749    45740       -9     
- Misses       4371     4380       +9

Flag	Coverage Δ
#multiple	`89.07% <ø> (ø)`	⬆️
#single	`40.32% <ø> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.75% <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 b4375bd...af55349. Read the comment docs.

codecov · 2017-11-03T14:58:09Z

Codecov Report

❗ No coverage uploaded for pull request base (master@cd80f08). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #18086   +/-   ##
=========================================
  Coverage          ?   91.38%           
=========================================
  Files             ?      163           
  Lines             ?    50065           
  Branches          ?        0           
=========================================
  Hits              ?    45752           
  Misses            ?     4313           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`89.19% <100%> (?)`
#single	`40.36% <50%> (?)`

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.48% <100%> (ø)`
pandas/tseries/frequencies.py	`96.01% <100%> (ø)`

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 cd80f08...3fa05bd. Read the comment docs.

jreback · 2017-11-04T23:35:38Z

pandas/_libs/tslibs/conversion.pyx

@@ -782,3 +782,130 @@ cdef inline str _render_tstamp(int64_t val):
    """ Helper function to render exception messages"""
    from pandas._libs.tslib import Timestamp
    return str(Timestamp(val))
+


is there a reason for the localization functions to exists here rather than in timezones.pyx?

Really want to limit the points of contact with pandas_datetimestruct and the ensuing dependencies.

jbrockmendel · 2017-11-05T16:39:30Z

BTW, once this and #18059 are closed we can cut/paste all of Timestamp into its own file.

jreback · 2017-11-07T13:39:55Z

rebase

jreback · 2017-11-07T15:22:05Z

pandas/_libs/tslibs/conversion.pyx

@@ -355,26 +365,16 @@ cdef inline void _localize_tso(_TSObject obj, object tz):
        # static/pytz/dateutil specific code
        if is_fixed_offset(tz):
            # statictzinfo
-            if len(deltas) > 0 and obj.value != NPY_NAT:


you are eliminating the not len(deltas) case

Right. get_dst_info never returns an empty array in the is_fixed_offset case.

More specifically: it always returns a length-1 array.

hmm can u add an assert to that effect then

OK. As long as we're asserting things, I think it is the case that in _localize_tso the input obj will always have obj.tzinfo is None; can we make that official? Even in the docstring would be a win.

asserts in internal code are a good thing
happy to add; they are just laying out the assumptions explicitly
don’t go crazy but a reader of the code will appreciate

Great. That'll close out one of the tslibs-todo questions. The last one since we're on a role: at the moment all calls to tz_convert or tz_convert_single have at least one of tz1 or tz2 being utc. Will that always be the case? If so that'll close [ill have to look it up].

iiuc i think so
we never convert from a time zone to another one directly
always pass thru utc

Excellent. I'll ping you on the appropriate bug-issue to close that out; don't want to derail this.

jreback · 2017-11-07T15:23:04Z

pandas/_libs/tslibs/conversion.pyx

+    return dtstruct_to_dt64(dts)
+
+
+def dates_normalized(ndarray[int64_t] stamps, tz=None):


we have date_normalized AND date_normalized?

Yep! One is a "normal this date" and the other is "is this date normalized?" As usual I'm not averse to changing that, but prefer to do so separately.

just change it to is_date_normalized then
much more clear

jreback · 2017-11-07T17:52:32Z

pandas/_libs/tslib.pyx

-    tz_convert_single)
+from tslibs.conversion import (tz_localize_to_utc,
+                               tz_convert_single, date_normalize)
+from tslibs.conversion import pydt_to_i8, tz_convert  # noqa


is there a reason these are noqa? simply import these where needed

jreback · 2017-11-07T17:53:41Z

pandas/_libs/tslibs/conversion.pyx

+    return dtstruct_to_dt64(dts)
+
+
+def are_dates_normalized(ndarray[int64_t] stamps, tz=None):


is_dates_normalized

it doesn’t follow our convention

technically this could be called
is_date_array_normalized

jreback · 2017-11-07T18:12:36Z

pandas/_libs/tslib.pyx

-
-    return ts.value
-
-
 def i8_to_pydt(int64_t i8, object tzinfo=None):
    """


should prob move this one to conversion

i8_to_pydt to be with the corresponding inverse (and import back here if needed)

Moving. AFAIK this isn't used anywhere.

ok can clean that up later

jreback · 2017-11-08T02:44:11Z

pandas/_libs/tslibs/conversion.pyx

+    """
+    Convert to int64 representation compatible with numpy datetime64; converts
+    to UTC
+    """


can type int64_t

jreback · 2017-11-08T02:45:50Z

pandas/_libs/tslibs/conversion.pyx

+@cython.wraparound(False)
+@cython.boundscheck(False)
+def date_normalize(ndarray[int64_t] stamps, tz=None):
+    cdef:


can u add a doc string

jreback · 2017-11-08T02:46:04Z

pandas/_libs/tslibs/conversion.pyx

+@cython.wraparound(False)
+@cython.boundscheck(False)
+cdef _normalize_local(ndarray[int64_t] stamps, object tz):
+    cdef:


jreback · 2017-11-08T02:46:20Z

pandas/_libs/tslibs/conversion.pyx

+    return dtstruct_to_dt64(dts)
+
+
+def are_dates_normalized(ndarray[int64_t] stamps, tz=None):


jreback · 2017-11-09T13:28:28Z

pandas/_libs/tslibs/conversion.pyx

+
+def i8_to_pydt(int64_t i8, object tzinfo=None):
+    """
+    Inverse of pydt_to_i8


can you add parameters and returns to the doc-strings

jreback · 2017-11-09T13:28:48Z

pandas/_libs/tslibs/conversion.pyx

+    """
+    Inverse of pydt_to_i8
+    """
+    from pandas import Timestamp


shouldn't this return a datetime.datetime? (exactly, and not a Timestamp)

I guess to make the name more accurate. This is never used, only moved because you asked.

if this is not used, then just remove

jreback · 2017-11-09T13:29:24Z

pandas/_libs/tslibs/conversion.pyx

+    """
+    Normalize each of the (nanosecond) timestamps in the given array by
+    rounding down to the beginning of the day (i.e. midnight).  If `tz`
+    is not None, then this is midnight for this timezone.


can you add parameters to doc-strring

jreback · 2017-11-09T13:29:30Z

pandas/_libs/tslibs/conversion.pyx

+    Normalize each of the (nanosecond) timestamps in the given array by
+    rounding down to the beginning of the day (i.e. midnight) for the
+    given timezone `tz`.
+


jreback · 2017-11-09T13:30:08Z

pandas/_libs/tslibs/conversion.pyx

+
+@cython.wraparound(False)
+@cython.boundscheck(False)
+cdef ndarray[int64_t] _normalize_local(ndarray[int64_t] stamps, object tz):


I think you changed the name? ok to de-privatize and rename to normalize_local_timestamps. I prefer more readable function names fyi.

Didn't change the name, no.

jreback · 2017-11-09T13:30:35Z

pandas/_libs/tslibs/conversion.pyx

+    return result
+
+
+cdef inline int64_t _normalized_stamp(pandas_datetimestruct *dts) nogil:


doc-string and de-privatize

It is private. Not importable.

jreback · 2017-11-09T13:30:55Z

pandas/_libs/tslibs/conversion.pyx

+    Check if all of the given (nanosecond) timestamps are normalized to
+    midnight, i.e. hour == minute == second == 0.  If the optional timezone
+    `tz` is not None, then this is midnight for this timezone.
+


add parameters in doc-string

good name

jbrockmendel · 2017-11-09T15:57:04Z

On rebasing I noticed a bunch of old scripts removed. Nice!

…libs-conversion11

jbrockmendel · 2017-11-12T18:47:19Z

Just tried rebasing. It seemed to work locally, but pushing didn't trigger CI. So did "git pull upstream master" too. I don't think there's any problems, just a heads up.

jreback · 2017-11-12T18:49:28Z

Just tried rebasing. It seemed to work locally, but pushing didn't trigger CI. So did "git pull upstream master" too. I don't think there's any problems, just a heads up.

of course it does. you need to git push yourbranch yourupstream -f

further you really really should squash / fixup these, no need for all of these commits
git rebase -i yourupstream

jreback · 2017-11-12T18:50:03Z

pandas/_libs/groupby.pyx

@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-


don't add extraneous things, ok for now

This was a dummy commit to force the CI on a TestClipboard failure.

jreback · 2017-11-12T18:50:58Z

pandas/_libs/tslibs/conversion.pyx

+    """
+    Inverse of pydt_to_i8
+    """
+    from pandas import Timestamp


if this is not used, then just remove

jreback · 2017-11-12T18:54:48Z

pandas/_libs/tslibs/conversion.pyx

@@ -90,6 +89,35 @@ cdef class _TSObject:
            return self.value


+cpdef int64_t pydt_to_i8(object pydt) except? -1:
+    """


this looks like it has exactly 2 uses, I would much rather
Timestamp(...).value and remove this routine (and remove the tests for it)

(pandas) bash-3.2$ grep -r pydt_to_i8 pandas/ --include '*.py' --include '*.pyx' pandas//_libs/tslib.pyx:cpdef pydt_to_i8(object pydt): pandas//_libs/tslib.pyx: Inverse of pydt_to_i8 pandas//_libs/tslibs/offsets.pyx:from pandas._libs.tslib import pydt_to_i8, monthrange pandas//_libs/tslibs/offsets.pyx: i8 = pydt_to_i8(dt) pandas//core/indexes/datetimes.py: return np.int64(libts.pydt_to_i8(key)).view(_NS_DTYPE) pandas//tests/scalar/test_timestamp.py: assert tslib.pydt_to_i8(result) == expected pandas//tests/scalar/test_timestamp.py: assert tslib.pydt_to_i8(result) == expected pandas//tests/scalar/test_timestamp.py: assert tslib.pydt_to_i8(result) == expected_tz pandas//tests/scalar/test_timestamp.py: assert tslib.pydt_to_i8(result) == expected_tz pandas//tests/scalar/test_timestamp.py: assert tslib.pydt_to_i8(result) == expected_utc pandas//tests/scalar/test_timestamp.py: assert tslib.pydt_to_i8(result) == expected pandas//tests/scalar/test_timestamp.py: assert tslib.pydt_to_i8(result) == expected pandas//tests/scalar/test_timestamp.py: assert tslib.pydt_to_i8(result) == expected_tz pandas//tests/scalar/test_timestamp.py: assert tslib.pydt_to_i8(result) == expected_tz pandas//tests/scalar/test_timestamp.py: assert tslib.pydt_to_i8(result) == expected_utc

jbrockmendel · 2017-11-12T19:04:38Z

Removed pydt_to_i8 per request. Holding off on removing pydt_to_i8 pending profiling the effects in offsets.pyx.

jreback · 2017-11-12T19:25:52Z

Removed pydt_to_i8 per request. Holding off on removing pydt_to_i8 pending profiling the effects in offsets.pyx.

typo! but ok.

jreback · 2017-11-12T19:27:28Z

ping on green.

jreback · 2017-11-12T21:04:47Z

thanks!

closes pandas-dev#17944

gfyoung added the Clean label Nov 3, 2017

jreback reviewed Nov 4, 2017

View reviewed changes

jreback requested changes Nov 7, 2017

View reviewed changes

jbrockmendel mentioned this pull request Nov 7, 2017

move implementation of Timedelta to tslibs.timedeltas #18085

Merged

4 tasks

jreback requested changes Nov 8, 2017

View reviewed changes

jreback requested changes Nov 9, 2017

View reviewed changes

jbrockmendel mentioned this pull request Nov 11, 2017

Tslibs resolution #18034

Merged

4 tasks

jbrockmendel added 12 commits November 12, 2017 10:39

Move normalization funcs up to conversion

665083f

fixup import typo

5481d7b

whitespace fixup

9db5568

edits per reviewer request

57e6204

assert obj.tzinfo is None inbound

1c7a1b1

update imports

8192f87

declare pydt_to_i8 in pxd

7b7e719

typo fixup

3c7e5aa

edits per reviewer request

ec6757f

remove unallowed type declaration

07c9594

edits per reviewer request

505f740

dummy commit to force CI

e7456ce

jbrockmendel force-pushed the tslibs-conversion11 branch from 76866bd to e7456ce Compare November 12, 2017 18:43

Merge branch 'master' of https://github.com/pandas-dev/pandas into ts…

693bb49

…libs-conversion11

jbrockmendel added 2 commits November 12, 2017 10:48

remove duplicate declaration

69d0865

whitespace fixup

84594cb

jreback requested changes Nov 12, 2017

View reviewed changes

remove i8_to_pydt per request

3fa05bd

jreback added this to the 0.22.0 milestone Nov 12, 2017

jreback approved these changes Nov 12, 2017

View reviewed changes

jreback merged commit 9e3ad63 into pandas-dev:master Nov 12, 2017

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

Move normalization funcs up to conversion (pandas-dev#18086)

358056b

closes pandas-dev#17944

jbrockmendel deleted the tslibs-conversion11 branch December 8, 2017 19:38

		return dtstruct_to_dt64(dts)


		def dates_normalized(ndarray[int64_t] stamps, tz=None):

		return dtstruct_to_dt64(dts)


		def are_dates_normalized(ndarray[int64_t] stamps, tz=None):

		return result


		cdef inline int64_t _normalized_stamp(pandas_datetimestruct *dts) nogil:

Move normalization funcs up to conversion #18086

Move normalization funcs up to conversion #18086

Conversation

jbrockmendel commented Nov 3, 2017

gfyoung commented Nov 3, 2017

codecov bot commented Nov 3, 2017

Codecov Report

codecov bot commented Nov 3, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Nov 5, 2017

jreback commented Nov 7, 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

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

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

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

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 Nov 9, 2017

jbrockmendel commented Nov 12, 2017

jreback commented Nov 12, 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

jbrockmendel commented Nov 12, 2017

jreback commented Nov 12, 2017

jreback commented Nov 12, 2017

jreback commented Nov 12, 2017

codecov bot commented Nov 3, 2017 •

edited

Loading

jbrockmendel Nov 7, 2017 •

edited

Loading