Skip to content
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

Move FrequencyInferer out of libresolution #21992

Merged
merged 12 commits into from
Jul 25, 2018
Merged

Conversation

jbrockmendel
Copy link
Member

Discussed briefly, FrequencyInferer doesn't benefit much from cython, isn't needed elsewhere in tslibs, and with this move loses the dependency on khash.

@@ -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)
Copy link
Contributor

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?

Copy link
Member Author

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.

delta = self.deltas[0]
if libresolution.is_multiple(delta, _ONE_DAY):
return self._infer_daily_rule()
else:
Copy link
Contributor

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
Copy link

codecov bot commented Jul 20, 2018

Codecov Report

Merging #21992 into master will increase coverage by 0.01%.
The diff coverage is 98.05%.

Impacted file tree graph

@@            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
Flag Coverage Δ
#multiple 90.42% <98.05%> (+0.02%) ⬆️
#single 42.31% <74.67%> (+0.09%) ⬆️
Impacted Files Coverage Δ
pandas/tseries/frequencies.py 97.08% <98.05%> (+1.17%) ⬆️

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 2d0c961...ebb6d52. Read the comment docs.

@jreback jreback added the Clean label Jul 20, 2018
@jreback jreback added this to the 0.24.0 milestone Jul 20, 2018
Copy link
Contributor

@jreback jreback left a 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

@@ -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:
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Member Author

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.

return len(self.deltas_asi8) == 1

def get_freq(self): # noqa:F811
if not self.is_monotonic or not self.index.is_unique:
Copy link
Contributor

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

@jreback jreback added the Frequency DateOffsets label Jul 20, 2018
Copy link
Contributor

@jreback jreback left a 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):
Copy link
Contributor

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:
Copy link
Contributor

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
Copy link
Contributor

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):
Copy link
Contributor

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

@jreback jreback merged commit e0b81d4 into pandas-dev:master Jul 25, 2018
@jbrockmendel jbrockmendel deleted the dereso branch July 26, 2018 16:23
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants