-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
Move FrequencyInferer out of libresolution #21992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
pandas/_libs/tslibs/resolution.pyx
Outdated
@@ -399,245 +358,14 @@ cdef object month_position_check(fields, weekdays): | |||
return None | |||
|
|||
|
|||
cdef inline bint _is_multiple(int64_t us, int64_t mult): | |||
@cython.returns(bint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't you just type it and maybe cpdef?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't called from cython, so no use for cpdef (and cython won't allow def bint ...
). For that matter might as well just move it along with FrequencyInferer.
pandas/tseries/frequencies.py
Outdated
delta = self.deltas[0] | ||
if libresolution.is_multiple(delta, _ONE_DAY): | ||
return self._infer_daily_rule() | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the else & dedent
Codecov Report
@@ Coverage Diff @@
## master #21992 +/- ##
==========================================
+ Coverage 92% 92.02% +0.01%
==========================================
Files 170 170
Lines 50553 50705 +152
==========================================
+ Hits 46510 46659 +149
- Misses 4043 4046 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just some doc-strings
pandas/_libs/algos.pyx
Outdated
@@ -71,6 +76,30 @@ class NegInfinity(object): | |||
__ge__ = lambda self, other: isinstance(other, NegInfinity) | |||
|
|||
|
|||
cpdef ndarray[int64_t, ndim=1] unique_deltas(ndarray[int64_t] arr): | |||
cdef: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a doc-string
|
||
import pandas._libs.tslibs.frequencies as libfreqs | ||
from pandas._libs.tslibs.frequencies import ( # noqa, semi-public API | ||
get_freq, get_base_alias, get_to_timestamp_base, get_freq_code, | ||
FreqGroup, | ||
is_subperiod, is_superperiod) | ||
from pandas._libs.tslibs.ccalendar import MONTH_ALIASES, int_to_weekday | ||
import pandas._libs.tslibs.resolution as libresolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe do one or the other here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe do one or the other here?
Other modules import from here. Agreed that we need to clean that up, would prefer to do that in one of the periodic pure-cleanup PRs.
pandas/tseries/frequencies.py
Outdated
return len(self.deltas_asi8) == 1 | ||
|
||
def get_freq(self): # noqa:F811 | ||
if not self.is_monotonic or not self.index.is_unique: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a docstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. some comments, pls follow up.
@@ -71,6 +76,42 @@ class NegInfinity(object): | |||
__ge__ = lambda self, other: isinstance(other, NegInfinity) | |||
|
|||
|
|||
cpdef ndarray[int64_t, ndim=1] unique_deltas(ndarray[int64_t] arr): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as an aside I think we usually call this diff elsewhere, so maybe can share code, and might thing about renaming (future PR to think about)
|
||
|
||
cdef object month_position_check(fields, weekdays): | ||
def month_position_check(fields, weekdays): | ||
cdef: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc-string in future, does this need cdef?
|
||
class _FrequencyInferer(object): | ||
""" | ||
Not sure if I can avoid the state machine here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in future, pls fix the doc-string
@@ -269,3 +283,246 @@ def infer_freq(index, warn=True): | |||
|
|||
inferer = _FrequencyInferer(index, warn=warn) | |||
return inferer.get_freq() | |||
|
|||
|
|||
class _FrequencyInferer(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would not object to de-privatizing this
Discussed briefly, FrequencyInferer doesn't benefit much from cython, isn't needed elsewhere in tslibs, and with this move loses the dependency on khash.