Rolling apply args#689
Conversation
📝 WalkthroughWalkthroughThe rolling ChangesRolling apply with args and kwargs support
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hey @andreas-wuestefeld, I had a few minutes before going home so I took a stab at the changes mentioned in #682. Mind taking a look to see if this will do what you need to simplify your other PRs? |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dascore/proc/rolling.py (1)
107-131:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against
axiskwarg collisions in_NumpyPatchRoller.apply.
rolling.applycurrently callsfunction(..., axis=-1, **kwargs). If the call site passesaxisin**kwargs, Python raises a duplicate-keywordTypeError(got multiple values for keyword argument 'axis'). Rejectaxisexplicitly with aParameterErrorbefore invokingfunction.Suggested patch
def apply(self, function, *args, **kwargs): """ {apply_description} Notes ----- The provided function must accept an ``axis`` argument. """ + if "axis" in kwargs: + msg = "`axis` is managed internally for rolling.apply; do not pass `axis`." + raise ParameterError(msg) # TODO look at replacing this with a call to `as_strided` that # accounts for strides. slide_view = np.lib.stride_tricks.sliding_window_view( self.patch.data, self.window, self.axis, ) @@ - raw = function(trimmed_slide_view, *args, axis=-1, **kwargs).astype(np.float64) + raw = function(trimmed_slide_view, *args, axis=-1, **kwargs).astype(np.float64) out = self._pad_roll_array(raw)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dascore/proc/rolling.py` around lines 107 - 131, In _NumpyPatchRoller.apply, guard against duplicate 'axis' by checking if 'axis' is present in the passed **kwargs before calling function(..., axis=-1, **kwargs); if 'axis' in kwargs raise ParameterError (use the project ParameterError type) with a clear message (e.g. "axis kwarg is not allowed; rolling.apply sets axis internally"), then proceed to call function with axis=-1; add this check just before the line that calls function in apply.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@dascore/proc/rolling.py`:
- Around line 107-131: In _NumpyPatchRoller.apply, guard against duplicate
'axis' by checking if 'axis' is present in the passed **kwargs before calling
function(..., axis=-1, **kwargs); if 'axis' in kwargs raise ParameterError (use
the project ParameterError type) with a clear message (e.g. "axis kwarg is not
allowed; rolling.apply sets axis internally"), then proceed to call function
with axis=-1; add this check just before the line that calls function in apply.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4be00770-4dff-485c-adb9-ee435dc7e71f
📒 Files selected for processing (2)
dascore/proc/rolling.pytests/test_proc/test_rolling.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #689 +/- ##
=======================================
Coverage 99.93% 99.93%
=======================================
Files 135 135
Lines 11704 11737 +33
=======================================
+ Hits 11696 11729 +33
Misses 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
andreas-wuestefeld
left a comment
There was a problem hiding this comment.
This looks like something I would have needed a while ago :-)
Not tested yet, will try over the weekend
|
OK, let's merge for now. If you decide the other api is better we can always do a follow up PR before the next release. |
Added STALTA, FBE, and Kurtosis transforms This works now also the with latest upgrade of the "rolling" function to allow for arguments being passed #689 numba implementation to speed up significantly --------- Co-authored-by: Derrick Chambers <d-chambers@users.noreply.github.com>
Description
This PR updates rolling
applyso custom functions can receive additional positional and keyword arguments directly from theapplycall.For example:
The change supports both rolling engines:
This also removes the unused internal func_kwargs field from _PatchRollerInfo, since function arguments are now supplied per apply call instead of stored on the roller object.
The shared apply docstring now documents the arguments and includes an example. Tests were added for numpy and pandas rolling apply with extra positional and keyword arguments.
Superceeds #682.
Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
applymethod now accepts extra positional and keyword arguments to pass to user-provided functions.Documentation
Tests