-
Notifications
You must be signed in to change notification settings - Fork 3
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
Quantile multiyear (part 1) #86
Conversation
Sorry, another mathsy fiddly one that I'm only doing to make the function slightly more intuitive. Part 2, where I actually add the wrapper to this, is coming soon. |
All good will take a look Monday (under the pump at the moment) |
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.
Indeed a fiddly one, which docs or examples should I be looking at to understand this?
tests/integration/crunchers/test_cruncher_quantile_rolling_windows.py
Outdated
Show resolved
Hide resolved
tests/integration/crunchers/test_cruncher_quantile_rolling_windows.py
Outdated
Show resolved
Hide resolved
tests/integration/crunchers/test_cruncher_quantile_rolling_windows.py
Outdated
Show resolved
Hide resolved
tests/integration/crunchers/test_cruncher_quantile_rolling_windows.py
Outdated
Show resolved
Hide resolved
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.
Looks good in general. Longer suggestions in #90
One other question, is it possible to eliminate the code duplication between QuantileRollingWindows
and rolling_window_find_quantiles
?
Interesting question. We could, but it's currently being used as a check to ensure that nothing untoward happens to the data in a test for the QRW cruncher. Although they were originally designed for different datatypes, they've pretty much drifted into being the same code now. |
I’d rewrite the test and remove the duplication. Better practice and makes long term maintenance easier |
Pull request
Please confirm that this pull request has done the following:
CHANGELOG.rst
addedAdding to CHANGELOG.rst
Please add a single line in the changelog notes similar to one of the following: