standalone implementation of ccalendar #18540

jbrockmendel · 2017-11-28T03:51:32Z

For the moment only updated cimports in fields.

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

…libs-ccalendar

codecov · 2017-11-28T05:53:57Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #18540      +/-   ##
==========================================
- Coverage   91.35%   91.33%   -0.03%     
==========================================
  Files         164      164              
  Lines       49801    49801              
==========================================
- Hits        45494    45484      -10     
- Misses       4307     4317      +10

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

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️
pandas/core/indexes/datetimes.py	`95.61% <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 88ab693...d91ec05. Read the comment docs.

codecov · 2017-11-28T05:54:25Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #18540      +/-   ##
==========================================
- Coverage   91.59%   91.57%   -0.02%     
==========================================
  Files         153      153              
  Lines       51212    51212              
==========================================
- Hits        46908    46898      -10     
- Misses       4304     4314      +10

Flag	Coverage Δ
#multiple	`89.43% <ø> (-0.01%)`	⬇️
#single	`40.67% <ø> (-0.11%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.81% <0%> (-0.1%)`	⬇️
pandas/core/indexes/datetimes.py	`95.59% <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 695e893...6b245cf. Read the comment docs.

jreback · 2017-11-28T11:10:51Z

what PR was this originally implemented in?

jreback · 2017-11-28T11:13:17Z

pandas/_libs/tslibs/ccalendar.pyx

+
+@cython.wraparound(False)
+@cython.boundscheck(False)
+cpdef monthrange(int64_t year, Py_ssize_t month):


why do we need this as a function that returns a tuple. its it simply better to split into 2, e.g. monthday and dayofweek? its 2 functions, but then these become all nogil

Those two separate functions also exist here. There's not much reason to retain monthrange, you're right.

are you changing this in this PR?

jreback · 2017-11-28T11:13:57Z

pandas/_libs/tslibs/ccalendar.pyx

+
+    y -= m < 3
+    day = (y + y / 4 - y / 100 + y / 400 + sakamoto_arr[m - 1] + d) % 7
+    # convert to python day


add a link to python dayofweek docs (e.g. its sunday 0, -6)

jreback · 2017-11-28T11:14:40Z

pandas/_libs/tslibs/ccalendar.pyx

+    # convert to python day
+    return (day + 6) % 7
+
+


on get_week_of_year you have boundscheck/wraparound but not here, do we need in either?

Is this comment about is_leapyear? If so, is_leapyear doesn't do any array lookups, so those decorators are irrelevant.

jreback · 2017-11-28T11:15:36Z

pandas/_libs/tslibs/ccalendar.pxd

@@ -0,0 +1,14 @@
+# -*- coding: utf-8 -*-
+# cython: profile=False


is this only used in fields? no-where else?

Only implemented in fields. Haven't updated others yet. A couple places in offsets, a few more fastpaths in Timestamp.

jbrockmendel · 2017-11-28T15:08:47Z

what PR was this originally implemented in?

#18489, but that was reverted as out-of-scope

…libs-ccalendar

jbrockmendel · 2017-11-28T15:51:53Z

Just pushed a commit that uses ccalendar functions to implement fastpaths for some of the Timestamp properties (ones not covered by #18539)

master

In [2]: ts = pd.Timestamp.now()
In [3]: %timeit ts.is_month_end
The slowest run took 34.08 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 26.8 µs per loop

PR

In [2]: ts = pd.Timestamp.now()
In [3]: %timeit ts.is_month_end
The slowest run took 27.70 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 1.19 µs per loop

jreback · 2017-11-29T00:16:46Z

pandas/_libs/tslibs/ccalendar.pyx

+
+@cython.wraparound(False)
+@cython.boundscheck(False)
+cpdef monthrange(int64_t year, Py_ssize_t month):


are you changing this in this PR?

jreback · 2017-11-29T00:17:11Z

pandas/_libs/tslibs/ccalendar.pyx

+
+
+@cython.wraparound(False)
+@cython.boundscheck(False)


you changing the boundscheck/wraparound in this PR?

Hadn't planned on it. Are they harmful?

prob not harmful but extra code and confusing. pls change.

Well in get_week_of_year there are array lookups, so disabling these checks should be perf-relevant, shouldn't they?

where are the array lookups?

Line 174 lookup in month_offset

…libs-ccalendar

jbrockmendel · 2017-11-29T01:03:53Z

are you changing [monthrange] in this PR?

Hadn't planned on it. Even if it shouldn't be used, ATM it still is used in a few places. Besides I wrote a really nice docstring.

jreback · 2017-11-29T11:58:18Z

Hadn't planned on it. Even if it shouldn't be used, ATM it still is used in a few places. Besides I wrote a really nice docstring.

what changed? we had this discussion about why returning a tuple is a bad idea

jorisvandenbossche · 2017-11-29T12:29:29Z

pandas/_libs/tslibs/ccalendar.pyx

+@cython.wraparound(False)
+@cython.boundscheck(False)
+@cython.cdivision
+cdef int dayofweek(int y, int m, int d) nogil:


It seems you are duplicating things from np_datetime.c ?

It seems you are duplicating things from np_datetime.c ?

@jorisvandenbossche think porting less than duplicating. The np_datetime.c version is never actually used in src/datetime (will remove old version in follow-up). As long as perf doesn't take a hit, the default should by python/cython right?

Sorry I don't understand your explanation. Previously it was imported it from np_datetime.c, and you reimplemented it here and changed that import. That seems like duplication rather than porting?

dayofweek is maybe not used in np_datetime.c itself, but is_leapyear is, so that one cannot just be removed there.

That seems like duplication rather than porting?

You're right. The intention is to remove the np_datetime.c version in a follow-up PR.

is_leapyear will be duplicated because it is used in np_datetime.c, as you noted. The idea is that np_datetime.c* is a heavy dependency (in practice tends to come with tslibs.np_datetime and np_datetime_strings.c) which can be replaced with a much lighter (and more pythonic) ccalendar dependency in some cases.

* The name overlap between np_datetime.c and tslibs.np_datetime is not my favorite.

I would rather include extra deps than duplicate code. mark this for a followup.

jbrockmendel · 2017-11-29T17:21:49Z

what changed? we had this discussion about why returning a tuple is a bad idea

Will remove.

…libs-ccalendar

jreback · 2017-12-04T11:30:28Z

pandas/_libs/tslibs/ccalendar.pyx

+
+
+@cython.wraparound(False)
+@cython.boundscheck(False)


where are the array lookups?

jreback · 2017-12-07T02:08:10Z

rebase and will look again

…libs-ccalendar

jreback · 2017-12-07T11:07:40Z

pandas/_libs/tslibs/ccalendar.pyx

+@cython.wraparound(False)
+@cython.boundscheck(False)
+@cython.cdivision
+cdef int dayofweek(int y, int m, int d) nogil:


I would rather include extra deps than duplicate code. mark this for a followup.

jbrockmendel added 18 commits November 26, 2017 15:13

implement ccalendar. only use it incrementally for profiling

4e942f5

Fix nameerror

6cbb4be

fixup signature

7f22c6d

next run with just is_leapyear changed

94557a7

then try with just get_days_in_month

ce8e3d5

fixup typo

344252e

use all of dayofweek, get_days_in_monht, is_leapyear in fields

a084fb3

try to avoid re-initializing sakamoto_arr

1b74668

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

bc4da9e

…libs-ccalendar

implement get_week_of_year

fdf280e

punctuation

f8cc945

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

0cfd9a4

…libs-ccalendar

use get_week_of_year

a29950b

whitesapce fixup

3d07a2a

whitesapce fixup

b16f443

comment

9ec475a

fixup typo

b221710

fix 12-->13

d91ec05

jreback added Clean Datetime Datetime data dtype labels Nov 28, 2017

jreback reviewed Nov 28, 2017

View reviewed changes

jreback requested changes Nov 28, 2017

View reviewed changes

flesh out docstring

8f78992

jbrockmendel added 2 commits November 28, 2017 07:50

implement ccalendar fastpaths for timestamp properties

32be4f4

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

3fda037

…libs-ccalendar

jreback approved these changes Nov 29, 2017

View reviewed changes

jreback requested changes Nov 29, 2017

View reviewed changes

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

03a854d

…libs-ccalendar

fixup comment typo

d0f8b13

jorisvandenbossche reviewed Nov 29, 2017

View reviewed changes

jbrockmendel added 4 commits November 29, 2017 09:22

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

505fa9e

…libs-ccalendar

remove monthrange per reviewer request

0add03e

remove monthrange from pxd

b66caa6

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

7cc0824

…libs-ccalendar

jreback requested changes Dec 4, 2017

View reviewed changes

pandas/_libs/tslibs/ccalendar.pyx

@cython.wraparound(False)

@cython.boundscheck(False)

Copy link

Contributor

jreback Dec 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where are the array lookups?

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

6b245cf

…libs-ccalendar

jreback approved these changes Dec 7, 2017

View reviewed changes

jreback added this to the 0.22.0 milestone Dec 7, 2017

jreback merged commit 3e506a3 into pandas-dev:master Dec 7, 2017

jbrockmendel deleted the tslibs-ccalendar branch December 7, 2017 15:57

jbrockmendel mentioned this pull request Feb 6, 2018

Implement get_day_of_year, tests #19555

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

standalone implementation of ccalendar #18540

standalone implementation of ccalendar #18540

jbrockmendel commented Nov 28, 2017

codecov bot commented Nov 28, 2017 •

edited

Loading

codecov bot commented Nov 28, 2017 •

edited

Loading

jreback commented Nov 28, 2017

jreback Nov 28, 2017

jbrockmendel Nov 28, 2017

jreback Nov 29, 2017

jreback Nov 28, 2017

jreback Nov 28, 2017

jbrockmendel Nov 28, 2017

jreback Nov 28, 2017

jreback Nov 28, 2017

jbrockmendel Nov 28, 2017

jbrockmendel commented Nov 28, 2017

jbrockmendel commented Nov 28, 2017

jreback Nov 29, 2017

jreback Nov 29, 2017

jbrockmendel Nov 29, 2017

jreback Nov 29, 2017

jbrockmendel Nov 29, 2017

jreback Dec 4, 2017

jbrockmendel Dec 4, 2017

jbrockmendel commented Nov 29, 2017

jreback commented Nov 29, 2017

jorisvandenbossche Nov 29, 2017

jbrockmendel Nov 29, 2017

jorisvandenbossche Nov 29, 2017

jbrockmendel Nov 29, 2017

jreback Dec 7, 2017

jbrockmendel commented Nov 29, 2017

jreback Dec 4, 2017

jreback commented Dec 7, 2017

jreback Dec 7, 2017

		@@ -0,0 +1,14 @@
		# -- coding: utf-8 --
		# cython: profile=False

standalone implementation of ccalendar #18540

standalone implementation of ccalendar #18540

Conversation

jbrockmendel commented Nov 28, 2017

codecov bot commented Nov 28, 2017 • edited Loading

Codecov Report

codecov bot commented Nov 28, 2017 • edited Loading

Codecov Report

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

jbrockmendel commented Nov 28, 2017

jbrockmendel commented Nov 28, 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

jbrockmendel commented Nov 29, 2017

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

jbrockmendel commented Nov 29, 2017

Choose a reason for hiding this comment

jreback commented Dec 7, 2017

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2017 •

edited

Loading

codecov bot commented Nov 28, 2017 •

edited

Loading