Separate out strptime.pyx from tslib #17342

jbrockmendel · 2017-08-26T02:20:58Z

This is the 2nd of an N part series of PRs to split tslib into independent modules.

At the moment there is a big chunk of code at the bottom of tslib that looks like it was pasted in from somewhere else. The header for that section of the file reads # Don't even ask. So I won't.

The new tslibs.strptime only used in one place: array_strptime is called in tools.datetimes. Other than that, nothing needs to be exposed, and nothing else in tslib relies on it.

The one function from tslib that strptime does need is _check_dts_bounds. This (mostly) moves that up to datetime.pxd, which both tslib and strptime already import anyway.

This is mostly a copy/paste of the existing functions+classes. I cleaned up a couple of places where variables used camelCase.

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

chris-b1 · 2017-08-26T10:36:19Z

Please don't make changes and move things in the same commit - it completely hides the changes when we're reviewing - thanks!

jreback · 2017-08-26T10:39:45Z

what is the rationale for moving this in the first place?

codecov · 2017-08-26T15:55:38Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #17342      +/-   ##
==========================================
- Coverage   91.03%   90.99%   -0.05%     
==========================================
  Files         162      162              
  Lines       49567    49567              
==========================================
- Hits        45125    45103      -22     
- Misses       4442     4464      +22

Flag	Coverage Δ
#multiple	`88.77% <ø> (-0.03%)`	⬇️
#single	`40.24% <ø> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/plotting/_converter.py	`63.23% <0%> (-1.82%)`	⬇️
pandas/core/frame.py	`97.72% <0%> (-0.1%)`	⬇️
pandas/core/indexes/datetimes.py	`95.23% <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 473a7f3...262da03. Read the comment docs.

codecov · 2017-08-26T15:55:45Z

Codecov Report

Merging #17342 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #17342      +/-   ##
==========================================
- Coverage   91.26%   91.24%   -0.02%     
==========================================
  Files         163      163              
  Lines       49776    49807      +31     
==========================================
+ Hits        45426    45447      +21     
- Misses       4350     4360      +10

Flag	Coverage Δ
#multiple	`89.04% <100%> (ø)`	⬆️
#single	`40.3% <50%> (-0.04%)`	⬇️

Impacted Files	Coverage Δ
pandas/_libs/__init__.py	`100% <ø> (ø)`	⬆️
pandas/core/tools/datetimes.py	`85.24% <100%> (+0.04%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/accessor.py	`93.65% <0%> (-6.35%)`	⬇️
pandas/core/frame.py	`97.73% <0%> (-0.15%)`	⬇️
pandas/core/series.py	`94.93% <0%> (ø)`	⬆️
pandas/core/indexes/category.py	`97.74% <0%> (ø)`	⬆️
pandas/core/categorical.py	`95.28% <0%> (ø)`	⬆️
pandas/io/parsers.py	`95.48% <0%> (+0.02%)`	⬆️
pandas/core/generic.py	`92.05% <0%> (+0.05%)`	⬆️
... and 2 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 d43aba8...5478aa2. Read the comment docs.

jbrockmendel · 2017-08-26T16:31:52Z

what is the rationale for moving this in the first place?

In order of "most specific to this PR" to "most general to this sequence of PRs":

It is not immediately obvious that strptime is independent of the rest of tslib. Separating it out makes both pieces easier to maintain. DAGs FTW.
tslib is huge and gets imported in a whole bunch of places, often times just for NaT. Making that import more light-weight is a win.
Over at dateutil we're talking about the next big push, and I'm advocating for making things more pandas-friendly. e.g. see tslib.dateutil_parse docstring """ lifted from dateutil to get resolution""". Having dateutil return this resolution information would make this workaround unnecessary, improving performance and code complexity.
To that end, I'm trying to centralize the date parsing code that happens in a handful of places around pandas (core.tools.datetimes, _libs.tslib, _libs/src/inference.pyx) and separate it out into module(s) with minimal intra-pandas dependencies.
A big part of the goal for this series of PRs is to disentangle the dependencies between tslib, period, and tseries, as tseries is a big performance bottleneck (in particular tseries.offsets.DateOffset, see Make some attributes of DateOffset read-only; add tests #17181 ).
See this comment in _libs.__init__:

# TODO
# period is directly dependent on tslib and imports python
# modules, so exposing Period as an alias is currently not possible

One of the big goals of this sequence of PRs is handling this.

jbrockmendel · 2017-09-06T16:08:04Z

Just pushed a commit to remove cython "non-standard" cython decorators and revert a camelCase fix. i.e. with the exception of a couple of flake8 whitespace changes, this should be a cut/paste of the existing functions.

jreback · 2017-09-07T11:51:37Z

this will need a rebase after #17422

jreback · 2017-09-14T10:20:40Z

pandas/_libs/src/datetime.pxd

+
+
+cdef inline check_dts_bounds(pandas_datetimestruct *dts):
+    cdef:


better to make this a bint, and just return True/False.

Then make another function which calls this one which actually raises the error.

The longer-term solution that I've implemented but not PRed is to move a bunch of datetime.pxd into a pyx file where OutOfBoundsDatetime can be defined. This also ends up clearing out a lot of the setup.py dependencies (orthogonal to this discussion, but still).

Is returning a bint actually any simpler than re-raising? It isn't obvious whether True means "error state is True" or "everything is OK is True", whereas raising is unambiguous.

That said, I'll do it your way.

yeah its ok, just don't like to muddle even more.

jreback · 2017-09-14T10:20:42Z

pandas/_libs/tslib.pyx

-        error = True
-
-    if error:
+    # Retaining this is a kludge because I haven't figured out how


jreback · 2017-09-14T10:23:11Z

setup.py

@@ -464,6 +464,8 @@ def pxd(name):
                   'pandas/_libs/src/period_helper.h',
                   'pandas/_libs/src/datetime.pxd']

+np_dtime_strs_srcs = ['pandas/_libs/src/datetime/np_datetime.c',


these sources are also used else where, just list them for now directly (e.g. like period.pyx and others do it). This doesn't add anything

OK. This will lead to flake8 warnings that I'm happy to ignore.

jreback · 2017-09-14T10:23:39Z

pandas/_libs/tslib.pyx

@@ -5356,317 +5074,4 @@ def shift_months(int64_t[:] dtindex, int months, object day=None):

 #----------------------------------------------------------------------


you can take this comment out :<

jreback · 2017-09-14T10:25:16Z

pandas/_libs/tslib.pyx

-
-# def _strptime_time(data_string, format="%a %b %d %H:%M:%S %Y"):
-#     return _strptime(data_string, format)[0]
+from tslibs.strptime import array_strptime


I don't think you need this at all; this is called in exactly 1 place in the code simply call it there. then strptime is decoupled.

Sounds great. I've been importing things back into tslib with #noqa to keep the namespace unchanged. Is that unnecessary more generally?

we might not be fully flaking this type of thing (or maybe we are and have to use noqa). I would like to remove non-essential imports though. These are ll private namespaces.

…o strptime

pep8speaks · 2017-09-14T15:58:25Z

Hello @jbrockmendel! Thanks for updating the PR.

In the file setup.py, following are the PEP8 issues :

Line 487:80: E501 line too long (84 > 79 characters)
Line 488:80: E501 line too long (94 > 79 characters)

Comment last updated on September 25, 2017 at 00:13 Hours UTC

jorisvandenbossche · 2017-09-15T13:23:07Z

pandas/_libs/tslibs/strptime.pyx

+
+FUNCTIONS:
+    _getlang -- Figure out what language is being used for the locale
+    strptime -- Calculates the time struct represented by the passed-in string


I know you copy pasted this, but this does not really suite for the module docstring, as this is only for part of the file.

So I would either move this inline to where those functions/classes are defined (as it was in tslib), or update it to reflect the actual file contents

If I understand correctly array_strptime is the only 'public' (for pandas, as used outside this file) function? If so, I would focus on that in the module docstring

There are a handful of style issues I'd like to circle back to (e.g. mixed_camel_Case), will add this to the list. For the time being I'm really eager to wrapping this up.

This is not really a style issue, but how you copy pasted things. If you want to clean-up later, I would just put this in the middle close of the file close to the relevant code for now (like it was in tslib)

Not sure I understand. Are you OK with this being addressed in a follow-up PR?

I would rather you fix the docs now. just move these docs down to where they are used, and put array_strptime up front

@jbrockmendel I see you integrated them into the class/functions docstrings, which is good, but I would still include a comment where LocaleTime starts (the place where previously the comment was) to make it clear that this part is some kind of vendored code (it just comes from the standard library: https://github.com/python/cpython/blob/master/Lib/_strptime.py)

jorisvandenbossche · 2017-09-15T13:24:32Z

There is some other code related to parsing strings in tslib as well. Does it make sense to move those as well?

jorisvandenbossche · 2017-09-15T13:25:09Z

There is some other code related to parsing strings in tslib as well. Does it make sense to move those as well?

Ah, I see there is already #17363 :-)

jreback · 2017-09-17T21:10:57Z

pandas/_libs/tslibs/strptime.pyx

+
+cdef set _nat_strings = set(['NaT', 'nat', 'NAT', 'nan', 'NaN', 'NAN'])
+
+


can we move both of these to util.pxd? to avoid repeating code

_nat_strings and what else?

checknull_with_nat, this seems like its repeating in lots of places.

jreback · 2017-09-23T19:46:39Z

needs a rebase

…rptime

jreback

generally ok with this. a couple of doc comments. if you can move checknull_with_nat (to avoid repeating code all over the place) to a .pxd great. if not, can do later. but pls make an issue in that case.

jreback · 2017-09-23T20:25:25Z

pandas/_libs/tslibs/strptime.pyx

+
+FUNCTIONS:
+    _getlang -- Figure out what language is being used for the locale
+    strptime -- Calculates the time struct represented by the passed-in string


I would rather you fix the docs now. just move these docs down to where they are used, and put array_strptime up front

jreback · 2017-09-23T20:25:51Z

pandas/_libs/tslibs/strptime.pyx

+
+cdef set _nat_strings = set(['NaT', 'nat', 'NAT', 'nan', 'NaN', 'NAN'])
+
+


checknull_with_nat, this seems like its repeating in lots of places.

jbrockmendel · 2017-09-23T23:06:48Z

checknull_with_nat, this seems like its repeating in lots of places

Ultimately both checknull_with_nat and _nat_strings belong in the not-yet-introduced tslibs.nattype. First choice would be to live with this bit of redundancy until that is ready. But putting them in util.pxd (or datetime.pxd) wouldn't be the end of the world.

jreback · 2017-09-24T12:43:30Z

pandas/_libs/tslibs/strptime.pyx

+
+cdef inline bint _checknull_with_nat(object val):
+    """ utility to check if a value is a nat or not """
+    return (val is None or


move this to the top and add a TODO to consolidate in future

jreback · 2017-09-24T12:44:39Z

pandas/_libs/tslibs/strptime.pyx

+        return 1 + days_to_week + day_of_week
+
+
+# def _strptime_time(data_string, format="%a %b %d %H:%M:%S %Y"):


remove commented

jreback · 2017-09-24T12:49:39Z

pandas/_libs/tslibs/strptime.pyx

+            if is_coerce:
+                iresult[i] = NPY_NAT
+                continue
+            else:


maybe worth it having a tiny extension util.pyx that houses things like this (and checknull for example)
something that we can import anywhere that isn't a dependency itself

actually i bet inference.pyx can be this extension

inference.pyx can be this extension

? inference.pyx depends on a bunch of stuff, including tslib.

The solution I've used locally is to put this into a tslibs.npy_dtime which has no dependencies.

ok, the name needs work though :>

ok, the name needs work though :>

OK, as long as it isn't another thing named datetime.

jreback · 2017-09-25T10:07:50Z

thanks!

jbrockmendel added 2 commits August 25, 2017 18:35

Separate out strptime.pyx

9b684a3

whitespace fixup

119dd22

gfyoung added Clean Internals Related to non-user accessible pandas implementation labels Aug 26, 2017

lint fixup

262da03

flake8 whitespace fixup

ac6a22a

jbrockmendel mentioned this pull request Aug 28, 2017

Separate parsing functions out from tslib #17363

Merged

4 tasks

dummy commit to force ci

9102cb7

jbrockmendel mentioned this pull request Sep 1, 2017

Refactor timezones functions out of tslib #17274

Closed

4 tasks

Remove cython decorators; revert camelCase fixes

89fcc2a

Add missing util cimport

73c4e7b

Merge branch 'master' into strptime

930d21a

jreback requested changes Sep 14, 2017

View reviewed changes

jbrockmendel added 2 commits September 14, 2017 08:56

Fixup per reviewer comments

bf3e618

Merge branch 'strptime' of https://github.com/jbrockmendel/pandas int…

50f3804

…o strptime

jorisvandenbossche reviewed Sep 15, 2017

View reviewed changes

flake8 whitespace fixup

1917bc1

jreback requested changes Sep 17, 2017

View reviewed changes

jbrockmendel mentioned this pull request Sep 23, 2017

Lock down kwargs in offsets signatures #17458

Merged

4 tasks

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

67c6664

…rptime

jreback requested changes Sep 23, 2017

View reviewed changes

Rearrange docstrings per reviewer request

5d8d1c4

jbrockmendel mentioned this pull request Sep 23, 2017

tslibs TODO list #17652

Closed

59 tasks

fixup missing util cimport

6cadd2b

jreback reviewed Sep 24, 2017

View reviewed changes

jbrockmendel added 2 commits September 24, 2017 09:51

rearrange per reviewer requests

6e9e44f

flake8 whitespace fixup

5478aa2

jreback approved these changes Sep 25, 2017

View reviewed changes

jreback merged commit 42195db into pandas-dev:master Sep 25, 2017

jreback added this to the 0.21.0 milestone Sep 25, 2017

jbrockmendel deleted the strptime branch October 30, 2017 16:23

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

Separate out strptime.pyx from tslib (pandas-dev#17342)

3ca0410

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

Separate out strptime.pyx from tslib (pandas-dev#17342)

7bde715



		cdef inline check_dts_bounds(pandas_datetimestruct *dts):
		cdef:

		@@ -5356,317 +5074,4 @@ def shift_months(int64_t[:] dtindex, int months, object day=None):

		#----------------------------------------------------------------------


		cdef set _nat_strings = set(['NaT', 'nat', 'NAT', 'nan', 'NaN', 'NAN'])

		return 1 + days_to_week + day_of_week


		# def _strptime_time(data_string, format="%a %b %d %H:%M:%S %Y"):

Separate out strptime.pyx from tslib #17342

Separate out strptime.pyx from tslib #17342

Conversation

jbrockmendel commented Aug 26, 2017

chris-b1 commented Aug 26, 2017

jreback commented Aug 26, 2017

codecov bot commented Aug 26, 2017

Codecov Report

codecov bot commented Aug 26, 2017 • edited Loading

Codecov Report

jbrockmendel commented Aug 26, 2017

jbrockmendel commented Sep 6, 2017

jreback commented Sep 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

pep8speaks commented Sep 14, 2017 • edited Loading

Comment last updated on September 25, 2017 at 00:13 Hours UTC

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

jorisvandenbossche commented Sep 15, 2017

jorisvandenbossche commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Sep 23, 2017

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

jbrockmendel commented Sep 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback Sep 24, 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

jreback commented Sep 25, 2017

codecov bot commented Aug 26, 2017 •

edited

Loading

pep8speaks commented Sep 14, 2017 •

edited

Loading

jreback Sep 24, 2017 •

edited

Loading