Move frequencies functions to cython #17746

jbrockmendel · 2017-10-02T17:26:27Z

Move some more functions from tseries.frequencies up to tslibs.frequencies, update imports in _libs.period to cimports

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

…libs-freqs3

jbrockmendel · 2017-10-02T19:19:51Z

The travis error is a flake8 complaint that get_freq is getting redefined in tseries.frequencies. AFAICT it is confused about scopes, because the original get_freq is a function imported at the module level and the other places get_freq is defined are both methods.

…libs-freqs3

codecov · 2017-10-10T14:56:13Z

Codecov Report

Merging #17746 into master will decrease coverage by 0.02%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master   #17746      +/-   ##
==========================================
- Coverage   91.54%   91.51%   -0.03%     
==========================================
  Files         148      148              
  Lines       48804    48680     -124     
==========================================
- Hits        44676    44550     -126     
- Misses       4128     4130       +2

Flag	Coverage Δ
#multiple	`89.88% <92.3%> (-0.03%)`	⬇️
#single	`41.68% <53.84%> (+0.05%)`	⬆️

Impacted Files	Coverage Δ
pandas/tseries/frequencies.py	`95.9% <92.3%> (+1.99%)`	⬆️
pandas/plotting/_converter.py	`65.22% <0%> (-1.74%)`	⬇️

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 4a8496b...8129b1c. Read the comment docs.

…libs-freqs3

jbrockmendel · 2017-10-17T19:35:15Z

Is the flake8 warning a deal-breaker?

bkmrkr · 2017-10-17T19:36:38Z

Please unsubscribe me

…

On Tue, Oct 17, 2017 at 3:35 PM, jbrockmendel ***@***.***> wrote: Is the flake8 warning a deal-breaker? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#17746 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJfa71N9T-MR_GNPgFhntVj9U0YQIkSks5stQF2gaJpZM4Pq-NZ> .

-- Greg Gurevich greggurevich@gmail.com cell: 1-917-504-7105

…libs-freqs3

jreback · 2017-10-31T19:29:48Z

pandas/_libs/tslib.pyx

@@ -92,6 +92,7 @@ from tslibs.timezones cimport (
    treat_tz_as_dateutil, treat_tz_as_pytz,
    get_timezone, get_utcoffset, maybe_get_tz,
    get_dst_info)
+from tslibs.frequencies cimport _get_rule_month


so this is prob unecessary

Looks like. Removing.

jreback · 2017-10-31T19:32:33Z

pandas/_libs/tslibs/frequencies.pxd

@@ -1,4 +1,10 @@
 # -*- coding: utf-8 -*-
 # cython: profile=False

+cpdef object _get_rule_month(object source, object default=*)
+


are these used anywhere but in period.pyx (for cython usage)?

There's a duplicate of it in tslibs.parsing that should be replaced with a cimport.

(things will be easier on my end if we can clean those up in a follow-up; im guessing you'll also suggest de-privatizing _get_rule_month)

that's fine, pls add to the list.

jreback · 2017-11-01T12:12:28Z

needs a rebase

…libs-freqs3

jreback · 2017-11-02T12:31:50Z

pandas/_libs/period.pyx

+from tslibs.frequencies cimport (
+    get_freq_code, get_base_alias, get_to_timestamp_base, _get_freq_str,
+    _get_rule_month)
+from tslibs.frequencies import _MONTH_NUMBERS


can't this be a cimport?

At the moment _MONTH_NUMBERS is not cdef'd. It is imported by tseries.frequencies, but not used there.

oh, then take it out

jreback

small comments

jreback · 2017-11-03T00:10:29Z

pandas/tseries/frequencies.py

@@ -609,7 +537,7 @@ def is_unique(self):
    def is_unique_asi8(self):
        return len(self.deltas_asi8) == 1

-    def get_freq(self):
+    def get_freq(self):  # noqa:F811  # Disable flake8 false-positive


For reasons unknown, flake8 was producing incorrect warnings about the function get_freq imported above being redefined. Here and on line 141.

…libs-freqs3

jbrockmendel · 2017-11-10T01:38:34Z

Travis error was an ImportError for moto. Rebase and push...

jreback · 2017-11-10T13:39:52Z

pandas/_libs/tslib.pyx

@@ -92,6 +92,7 @@ from tslibs.timezones cimport (
    treat_tz_as_dateutil, treat_tz_as_pytz,
    get_timezone, get_utcoffset, maybe_get_tz,
    get_dst_info)
+from tslibs.frequencies cimport _get_rule_month


jreback · 2017-11-10T13:40:58Z

pandas/tseries/frequencies.py

 from pandas._libs.tslib import Timedelta
-from pandas._libs.tslibs.frequencies import (  # noqa
+from pandas._libs.tslibs.frequencies import (  # noqa:F401


pls don't add a noqa, let's just remove the functions that are causing it

if there are public functions that should remain, put them in a separate import and indicate this

jreback · 2017-11-10T13:41:35Z

pandas/tseries/frequencies.py

@@ -149,7 +138,7 @@ def get_freq_group(cls, resostr):
        """
        return get_freq_group(cls.get_freq(resostr))

-    @classmethod
+    @classmethod  # noqa:F811  # Disable flake8 false-positive


pls don't use flake warnings like this

See discussion from 8 days ago:

For reasons unknown, flake8 was producing incorrect warnings about the function get_freq imported above being redefined. Here and on line 141.

The easiest way to make this these unnecessary is probably #18034.

jreback · 2017-11-10T13:43:07Z

pandas/tseries/frequencies.py

@@ -609,7 +537,7 @@ def is_unique(self):
    def is_unique_asi8(self):
        return len(self.deltas_asi8) == 1

-    def get_freq(self):
+    def get_freq(self):  # noqa:F811  # Disable flake8 false-positive


…libs-freqs3

jbrockmendel · 2017-12-11T16:33:24Z

Since I'm going to have to run asv umpteen times to get reasonable results, I'm inclined to make a separate PR that just does the de-privatizing+centraling of _MONTHS/_MONTH_ALIASES/... and get that out of the way, cut the diff here back down.

…libs-freqs3

jbrockmendel · 2017-12-16T18:12:28Z

I think this should be good to go.

jreback · 2017-12-20T12:49:41Z

pandas/_libs/tslibs/frequencies.pyx

+
+
+cpdef str get_freq_str(base, mult=1):
+    code = _reverse_period_code_map.get(base)


can you add a doc-string

jreback · 2017-12-20T12:50:01Z

pandas/_libs/tslibs/frequencies.pyx

+    return _base_and_stride(freqstr)[0]
+
+
+class FreqGroup(object):


can you move this class to the top

jreback · 2017-12-20T12:50:10Z

pandas/_libs/tslibs/frequencies.pyx

+    """
+    Return frequency code group used for base of to_timestamp against
+    frequency code.
+


add Parameters, Returns

jreback · 2017-12-20T12:50:24Z

pandas/_libs/tslibs/frequencies.pyx

+
+
+cpdef get_base_alias(freqstr):
+    """


add Parameters, Returns

return str? (or do we just use object)

jreback · 2017-12-20T12:51:06Z

pandas/_libs/tslibs/frequencies.pyx

+# Frequency comparison
+
+def is_subperiod(source, target):
+    """


jreback · 2017-12-20T12:52:19Z

pandas/_libs/tslibs/frequencies.pyx

+cpdef object get_rule_month(object source, object default='DEC'):
+    """
+    Return starting month of given freq, default is December.
+


jreback · 2017-12-20T12:52:47Z

pandas/tests/tseries/offsets/test_offsets.py

+
+from pandas._libs.tslibs.frequencies import (get_freq_code, get_freq_str,
+                                             _INVALID_FREQ_ERROR)
+from pandas.tseries.frequencies import (_offset_map,


_offset_map? future PR?

I think _offset_map is intended to be private, don't think its imported outside of tests. (Also if we ever make the immutable thing work, we'll be able to get rid of offset_map altogether)

jreback · 2017-12-20T12:53:41Z

pandas/tests/tseries/offsets/test_offsets.py

-from pandas.tseries.frequencies import (_offset_map, get_freq_code,
-                                        _get_freq_str, _INVALID_FREQ_ERROR,
+
+from pandas._libs.tslibs.frequencies import (get_freq_code, get_freq_str,


a test_libfrequencies? for testing the cython impl

jreback · 2017-12-20T12:54:55Z

pandas/tests/tseries/test_frequencies.py

@@ -7,6 +7,10 @@
 from pandas import (Index, DatetimeIndex, Timestamp, Series,
                    date_range, period_range)

+from pandas._libs.tslibs.frequencies import (_period_code_map, get_rule_month,


maybe move this to test_libfrequencies as seems to be mostly doing that. (could also do pandas/tests/tseries/frequencies/test_libfrequencies.py)

jreback · 2017-12-20T12:56:04Z

pandas/tests/tseries/test_frequencies.py

@@ -7,6 +7,10 @@
 from pandas import (Index, DatetimeIndex, Timestamp, Series,
                    date_range, period_range)

+from pandas._libs.tslibs.frequencies import (_period_code_map, get_rule_month,
+                                             _period_str_to_code,
+                                             _INVALID_FREQ_ERROR)


are these methods marked non-private (_period_st_to_code) used outside of libfrequencies? if so, de-privatize (and cpdef them). same with INVALID_FREQ_ERR

Mostly just in tests. If OK I'm going to save the remaining de-privatization and test_libfreqs for a follow-up. Too many active PRs and this one is ready for its AARP membership.

well I would rather get it right here. as we progress more and more the PR's by-definition get more fine grained and I care much more about the details. when just moving stuff, sure that doesn't matter.

_INVALID_FREQ_ERROR is used once in the non-cython frequencies. The others are used in tests but otherwise not outside libfreqs

…libs-freqs3

jreback · 2017-12-21T19:29:55Z

pandas/_libs/tslibs/frequencies.pyx

+cpdef str get_base_alias(freqstr):
+    """
+    Returns the base frequency alias, e.g., '5D' -> 'D'
+    """


can you add parameters/returns.

jreback · 2017-12-21T19:31:07Z

pandas/_libs/tslibs/frequencies.pyx

+        return FreqGroup.FR_DAY
+    if FreqGroup.FR_HR <= base <= FreqGroup.FR_SEC:
+        return FreqGroup.FR_SEC
+    return base


can you add a considtion for base < FR_NS. (and make these if/else).

the final entry should be a raise (as base is out of range).

jreback · 2017-12-21T19:31:18Z

pandas/_libs/tslibs/frequencies.pyx

+    >>> get_to_timestamp_base(get_freq_code('S')[0])
+    9000
+    """
+    if base < FreqGroup.FR_BUS:


jreback · 2017-12-21T19:33:35Z

pandas/tests/tseries/test_frequencies.py

@@ -7,6 +7,10 @@
 from pandas import (Index, DatetimeIndex, Timestamp, Series,
                    date_range, period_range)

+from pandas._libs.tslibs.frequencies import (_period_code_map, get_rule_month,
+                                             _period_str_to_code,
+                                             _INVALID_FREQ_ERROR)


well I would rather get it right here. as we progress more and more the PR's by-definition get more fine grained and I care much more about the details. when just moving stuff, sure that doesn't matter.

…libs-freqs3

jbrockmendel · 2018-01-04T17:39:59Z

It'd be nice to close this out since momentum for tslibs refactoring has waned.

jreback

lgtm. a couple of small fixes if you can do now, can defer larger ones (e.g. add more examples / parameterize) to later, but pls fix the Example and the _assert_depr test name

jreback · 2018-01-04T17:49:12Z

pandas/_libs/tslibs/frequencies.pyx

+    Returns
+    -------
+    is_subperiod : boolean
+    """


later on can you come back and add examples to functions w/o

jreback · 2018-01-04T17:49:22Z

pandas/_libs/tslibs/frequencies.pyx

+
+    Returns
+    -------
+    is_superperiod : boolean


jreback · 2018-01-04T17:49:37Z

pandas/_libs/tslibs/frequencies.pyx

+    -------
+    rule_month: object (usually string)
+
+    Example


-> Examples

jreback · 2018-01-04T17:50:16Z

pandas/tests/tseries/test_libfrequencies.py

+    result = get_rule_month('Q')
+    assert (result == 'DEC')
+    result = get_rule_month(offsets.QuarterEnd(startingMonth=12))
+    print(result == 'DEC')


can you remove the print

jreback · 2018-01-04T17:50:28Z

pandas/tests/tseries/test_libfrequencies.py

+                                             is_superperiod, is_subperiod)
+
+
+def test_get_rule_month():


later on parameterize (or now ok too)

jreback · 2018-01-04T17:51:13Z

pandas/tests/tseries/test_libfrequencies.py

+    assert (_period_str_to_code('Q-DEC') == 2000)
+    assert (_period_str_to_code('Q-FEB') == 2002)
+
+    def _assert_depr(freq, expected, aliases):


can you move this to the top and make a better name for it (and de-preivatize)

…libs-freqs3

jreback · 2018-01-05T00:34:47Z

pandas/tests/tseries/test_libfrequencies.py

+                                             is_superperiod, is_subperiod)
+
+
+def assert_aliases_deprecated(freq, expected, aliases):


do we have these listed in deprecations for removal? @gfyoung ?

we should blow these away in 0.23.0, can you create an issue an add to the list?

jreback · 2018-01-05T00:35:23Z

thanks @jbrockmendel

jbrockmendel added 2 commits October 2, 2017 08:56

Move tseries.frequencies funcs up to tslibs.frequencies

b77d09d

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

d551246

…libs-freqs3

jreback added the Frequency DateOffsets label Oct 3, 2017

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

cb227dc

…libs-freqs3

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

6532f76

…libs-freqs3

jbrockmendel mentioned this pull request Oct 18, 2017

PERF: enable caching on expensive offsets #17914

Closed

jbrockmendel added 2 commits October 19, 2017 21:32

more specific noqa, try to fix incorrect flake warning

b5198f3

disable flake8 false-positives

3bfbdf1

jbrockmendel mentioned this pull request Oct 25, 2017

PERF: Regressions since 0.20.3 #17861

Closed

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

4b8cb92

…libs-freqs3

jreback requested changes Oct 31, 2017

View reviewed changes

jbrockmendel added 3 commits November 1, 2017 08:22

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

da8369f

…libs-freqs3

flake8 fixup

74258c0

consolidate constants near top of file

823db82

jreback reviewed Nov 2, 2017

View reviewed changes

jreback requested changes Nov 3, 2017

View reviewed changes

jbrockmendel added 3 commits November 2, 2017 17:12

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

c1d8ec1

…libs-freqs3

remove unused variable

551f047

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

026056f

…libs-freqs3

jbrockmendel mentioned this pull request Nov 6, 2017

Remove out-of-date numpy.pxd; remove unused float16_t #18101

Closed

4 tasks

jbrockmendel added 2 commits November 9, 2017 09:27

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

925aca9

…libs-freqs3

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

92b1e9e

…libs-freqs3

jreback requested changes Nov 10, 2017

View reviewed changes

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

ae06978

…libs-freqs3

de-privatize get_rule_month

f60fe4d

jbrockmendel added 4 commits December 11, 2017 15:07

remove unused import

f250550

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

ae9f628

…libs-freqs3

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

db44aa5

…libs-freqs3

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

02e3217

…libs-freqs3

jreback requested changes Dec 20, 2017

View reviewed changes

jbrockmendel added 5 commits December 20, 2017 11:21

add types and docstrings

23a3588

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

3769095

…libs-freqs3

typo fixup bool-->bint

a7f063c

fix typing

3ee6e92

whitespace fixup

64ef864

jreback requested changes Dec 21, 2017

View reviewed changes

jbrockmendel added 4 commits December 23, 2017 18:08

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

56d7949

…libs-freqs3

implement test_libfrequencies per request

6d62e04

extend docstring per request

1d71924

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

730b21b

…libs-freqs3

jbrockmendel mentioned this pull request Dec 29, 2017

Make Series[datetime64] - pd.NaT behave like DatetimeIndex - pd.NaT #18960

Closed

4 tasks

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

010a7a5

…libs-freqs3

jreback requested changes Jan 4, 2018

View reviewed changes

jreback added this to the 0.23.0 milestone Jan 4, 2018

jbrockmendel added 2 commits January 4, 2018 10:19

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

a5d408e

…libs-freqs3

fix docstrings, rename _assert_depr

8129b1c

jreback approved these changes Jan 5, 2018

View reviewed changes

jreback reviewed Jan 5, 2018

View reviewed changes

jreback merged commit 77ec4e8 into pandas-dev:master Jan 5, 2018

jbrockmendel deleted the tslibs-freqs3 branch January 23, 2018 04:40



		cpdef str get_freq_str(base, mult=1):
		code = _reverse_period_code_map.get(base)

		is_superperiod, is_subperiod)


		def assert_aliases_deprecated(freq, expected, aliases):

Move frequencies functions to cython #17746

Move frequencies functions to cython #17746

Conversation

jbrockmendel commented Oct 2, 2017 • edited Loading

jbrockmendel commented Oct 2, 2017

codecov bot commented Oct 10, 2017 • edited Loading

Codecov Report

jbrockmendel commented Oct 17, 2017

bkmrkr commented Oct 17, 2017 via email

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

jreback commented Nov 1, 2017

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

jbrockmendel commented Nov 10, 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 Dec 11, 2017

jbrockmendel commented Dec 16, 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

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 Jan 4, 2018

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

jreback commented Jan 5, 2018

jbrockmendel commented Oct 2, 2017 •

edited

Loading

codecov bot commented Oct 10, 2017 •

edited

Loading