ENH: Rolling rank by gsiano · Pull Request #43338 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation52 Commits18 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
xref #9481
- closes Implement high performance rolling_rank #9481
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
rolling rank and percentile rank using skiplist
rolling rank and percentile rank using skiplist
remove unused variables, fix indentation, add comment to roll_rank()
don't fill output when nobs < minp
Hello @gsiano! 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 2021-09-13 18:29:13 UTC
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add this to the asvs as well.
will this close #9481 ? (can pull some examples fromt here)
as a followup (can create an issue), need tests / impl for groupby.rolling.rank (it should just work though)
Rolling rank ENH: Rolling rank
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pr @gsiano! Some comments on the cython code
raise MemoryError on skiplist_insert failure, destroy skiplist before re-init, and other minor changes
implement min, max, and average rank methods
I added the
min,max, andaveragerank methods. I still have to add thedenseandfirstmethods, and I'll look into adding the other flags fromDataFrame.rank
the first method doesn't seem relevant for rolling rank, and dense probably deserves a separate pr
I added the
min,max, andaveragerank methods. I still have to add thedenseandfirstmethods, and I'll look into adding the other flags fromDataFrame.rankthe
firstmethod doesn't seem relevant for rolling rank, anddenseprobably deserves a separate pr
+1 on pushing dense to a separate pr. ascending would be a nice flag to have ... I wonder if something like just adding the negative of the value to the skiplist would handle flipping the order
+1 on pushing
denseto a separate pr.ascendingwould be a nice flag to have ... I wonder if something like just adding the negative of the value to theskiplistwould handle flipping the order
That sounds like it will work. I'll try adding it.
added the ascending flag, various cleanups, expanded tests and asv benchmark
remove unimplemented rank types
reorder parameter list to match DataFrame.rank
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, can you add
- whatsnew note (1.4 enhacements), wouldn't object to a small example if you'd like (or a single note ok)
- add in the reference section: https://github.com/pandas-dev/pandas/blob/master/doc/source/reference/window.rst somewhere
- this is just for rolling, or works for expanding? (i think it should, can you add tests)
- i think we need a doc-string in rolling.py
added tests for Expanding. added doc strings and whatsnew note
looks good, can you add
- whatsnew note (1.4 enhacements), wouldn't object to a small example if you'd like (or a single note ok)
- add in the reference section: https://github.com/pandas-dev/pandas/blob/master/doc/source/reference/window.rst somewhere
- this is just for rolling, or works for expanding? (i think it should, can you add tests)
- i think we need a doc-string in rolling.py
I added a test for expanding and updated the docs and whatsnew file. Is there a way to render the docs locally so I can see how it looks?
| @pytest.mark.parametrize("ascending", [True, False]) |
|---|
| @pytest.mark.parametrize("test_data", ["default", "duplicates", "nans"]) |
| def test_rank(window, method, pct, ascending, test_data): |
| length = 1000 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Could we test with a smaller sample (e.g. 20) and adjust window accordingly?
| @pytest.mark.parametrize("ascending", [True, False]) |
|---|
| @pytest.mark.parametrize("test_data", ["default", "duplicates", "nans"]) |
| def test_rank(window, method, pct, ascending, test_data): |
| length = 1000 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comments, otherwise LGTM!
| int64_t nobs = 0, win |
|---|
| float64_t val |
| skiplist_t *skiplist |
| float64_t[::1] output = None |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| float64_t[::1] output = None |
|---|
| float64_t[::1] output |
NBD since doesn't affect correctness, but I find this clearer since None initialization usually used only when there's a path where the variable might not end up initialized. Also generates a bit less code :)
| else: |
|---|
| rank = NaN |
| if nobs >= minp: |
| output[i] = <float64_t>(rank) / nobs if percentile else rank |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the cast here necessary?
| ) |
|---|
| def rank( |
| self, |
| method: str = "average", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent this could be the same Literal you defined in aggregations.pyi? Maybe that could be aliased in pandas/_typing.py, so that then if we do something like add dense then we only need to change typing in one place.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah these should be consisteent across the rank methods (could be a followup to fix this)
| elif test_data == "duplicates": |
|---|
| ser = Series(data=np.random.choice(3, length)) |
| elif test_data == "nans": |
| ser = Series(data=np.random.choice([1.0, 0.25, 0.75, np.nan], length)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you throw an inf and -inf in here? Not really a special case I guess with how it's implemented in rolling_rank, but it is special cased in other rank algos and were some issues before with inf and nan, so covering here would be good
| elif test_data == "duplicates": |
|---|
| ser = Series(data=np.random.choice(3, length)) |
| elif test_data == "nans": |
| ser = Series(data=np.random.choice([1.0, 0.25, 0.75, np.nan], length)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above about inf
| .. ipython:: python |
| >>> s = pd.Series([1, 4, 2, 3, 5, 3]) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just put the commands; they will render during the doc-build (e.g. L100 w/o the '>>>' and not below)
| 5 2.0 |
|---|
| dtype: float64 |
| >>> s.expanding().rank() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could add a comment; this works for expanding as well (or kill the expanding, up 2 you)
| @@ -1139,6 +1141,120 @@ def roll_quantile(const float64_t[:] values, ndarray[int64_t] start, |
|---|
| return output |
| cdef enum RankType: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use the enums defined in pandas/_libs/algos.pyx? e.g. the TIEBREAK_AVERAGE. may need to move the to algox.pyd to import properly.
| ) |
|---|
| def rank( |
| self, |
| method: str = "average", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah these should be consisteent across the rank methods (could be a followup to fix this)
| @@ -1409,6 +1409,22 @@ def quantile(self, quantile: float, interpolation: str = "linear", **kwargs): |
|---|
| return self._apply(window_func, name="quantile", **kwargs) |
| def rank( |
| self, |
| method: str = "average", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here
add rolling rank tiebreakers dict
fix docs warnings - remove '>>>' from code block
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. small comment to consider for followup.
cc @mzeitlin11 over to you
| @@ -1139,6 +1143,122 @@ def roll_quantile(const float64_t[:] values, ndarray[int64_t] start, |
|---|
| return output |
| rolling_rank_tiebreakers = { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible to unify these with the same in algos.pyx?
oh see you approved, ok then.
thanks @gsiano very nice! pls create an issue for for the requested followups (PR as well if you can .....) keep em coming!