-
-
Notifications
You must be signed in to change notification settings - Fork 18.2k
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
CLN: Centralised _check_percentile #27584
Conversation
- Fixes GH27559. - Moved the _check_percentile method on NDFrame to algorithms as check_percentile. - Changed the references to _check_percentile in pandas/core/series.py and pandas/core/frame.py
Can you also update the groupby reference from the OP? |
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.
See above comment
Sorry I couldn't figure out what it was referring to. I didnt find any references to _check_percentile in groupby. Can you help me figure it out? |
Oh sorry misread that on my end - not there yet for groupby so I think OK for now |
pandas/core/algorithms.py
Outdated
@@ -1105,6 +1105,22 @@ def _get_score(at): | |||
return result | |||
|
|||
|
|||
def check_percentile(q): |
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 annotate this function? I think q
should just be float, Iterable[float]
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.
Return should be an ndarray
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.
Done.
pandas/core/algorithms.py
Outdated
""" | ||
|
||
msg = "percentiles should all be in the interval [0, 1]. " "Try {0} instead." | ||
q = np.asarray(q) |
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.
You can just create a new variable here q_arr
instead of reusing the argument; should help with some of the typing errors
Co-Authored-By: William Ayd <william.ayd@icloud.com>
ok will have to backport this as #27473 depends |
Hello @hedonhermdev! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-10-03 15:22:43 UTC |
@hedonhermdev seems like there's still a linting issue, and maybe some CI failures. |
@hedonhermdev can you update? |
Pushing to 1.0 |
@hedonhermdev can you rebase |
Nice idea but I think this has gone stale. @hedonhermdev please ping if you'd like to pick this back up |
Hey sorry for the disappearance, if possible I would like to work on it again. Can you reopen the pull request? |
Sure - thanks for the contribution |
- Fixes GH27559. - Moved the _check_percentile method on NDFrame to algorithms as check_percentile. - Changed the references to _check_percentile in pandas/core/series.py and pandas/core/frame.py Annotated check_percentile function. Update pandas/core/algorithms.py Co-Authored-By: William Ayd <william.ayd@icloud.com> Fixed typing error in check_percentile. Refactored docstring of check_percentile function. Fixed PEP8 issues.
pandas/core/algorithms.py
Outdated
@@ -1102,6 +1102,37 @@ def _get_score(at): | |||
return result | |||
|
|||
|
|||
def check_percentile(q: Union[float, Iterable[float]]) -> np.ndarray: |
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 this make more sense in validators?
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.
I think that's reasonable
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.
Should I shift it to utilts/_validators.py ?
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.
Yea let's do that
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 renaming it to validate_percentile be better?
pandas/core/algorithms.py
Outdated
@@ -1864,7 +1860,7 @@ def searchsorted(arr, value, side="left", sorter=None): | |||
else: | |||
value = array(value, dtype=dtype) | |||
elif not ( | |||
is_object_dtype(arr) or is_numeric_dtype(arr) or is_categorical_dtype(arr) | |||
is_object_dtype(arr) or is_numeric_dtype(arr) or is_categorical_dtype(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.
can you not add unrelated changes (all of this whitespace)
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.
Sorry for that.
pandas/util/_validators.py
Outdated
otherwise raises a ValueError. | ||
|
||
Parameters | ||
------- |
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.
Needs to be the length of Parameters.
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.
Oh, okay.
pandas/core/algorithms.py
Outdated
or is_period_dtype(dtype) | ||
or is_datetime64_any_dtype(dtype) | ||
or is_timedelta64_dtype(dtype) | ||
needs_i8_conversion(values) |
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 these changes?
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.
Sorry for these
Looks like a linting failure: https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=17752 Try running |
@hedonhermdev linting issue: https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=17804 In the future, I'd recommend using pre-commit as described in https://dev.pandas.io/docs/development/contributing.html#python-pep8-black |
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 (small comments). pls merge master and ping on green
@@ -1165,7 +1163,6 @@ def compute(self, method): | |||
|
|||
# slow method | |||
if n >= len(self.obj): | |||
|
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 revert these files as they are not actually changed
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.
isort fails if I don't commit them
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.
ok that's odd
pandas/util/_validators.py
Outdated
------ | ||
ValueError if percentiles are not in given interval([0, 1]). | ||
""" | ||
msg = "percentiles should all be in the interval [0, 1]. " "Try {0} instead." |
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.
Extra quotation marks before 'Try'
@jbrockmendel should I modify excel.py too? As I'm getting a linting issue because of it. |
It looks like it just wants you to revert the whitespace change in the excel.py file |
thanks @hedonhermdev |
* master: (22 commits) DOC: fix PR09,PR08 errors for pandas.Timestamp (pandas-dev#28739) WEB: Add diversity note to team.md (pandas-dev#28630) DOC: Minor fixes in pandas/testing.py docstring. (pandas-dev#28752) TST: port maybe_promote tests from pandas-dev#23982 (pandas-dev#28764) Bugfix/groupby datetime issue (pandas-dev#28569) reenable codecov (pandas-dev#28750) CLN: Centralised _check_percentile (pandas-dev#27584) DEPR: Deprecate Index.set_value (pandas-dev#28621) CLN: Fix typo in contributing.rst (pandas-dev#28761) Fixed docstring errors in pandas.period range and pandas.PeriodIndex (pandas-dev#28756) BUG: Fix TypeError raised in libreduction (pandas-dev#28643) DOC: Pandas.Series.drop docstring PR02 (pandas-dev#27976) (pandas-dev#28742) DOC: Fixed doctring errors PR08, PR09 in pandas.io (pandas-dev#28748) TST: Fix broken test cases where Timedelta/Timestamp raise (pandas-dev#28729) REF: Consolidate alignment calls in DataFrame ops (pandas-dev#28638) BUG: Fix dep generation (pandas-dev#28734) Added doctstring to fixture (pandas-dev#28727) DOC: Fixed PR06 docstrings errors in pandas.timedelta_range (pandas-dev#28719) replaced safe_import with a corresponding test decorator (pandas-dev#28731) BUG: Fix RangeIndex.get_indexer for decreasing RangeIndex (pandas-dev#28680) ...
Fixes CLN: centrailize _check_percentile #27559
Moved the _check_percentile method on NDFrame to algorithms as
check_percentile.
Changed the references to _check_percentile in pandas/core/series.py
and pandas/core/frame.py
closes CLN: centrailize _check_percentile #27559
tests added / passed
passes
black pandas
passes
git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry