Pass min_periods to rolling() in WindowFeatures - #1043
Open
VenishPaneliya wants to merge 1 commit into
Open
Conversation
WindowFeatures documents min_periods as a pandas rolling() passthrough
and stores it on the transformer, but transform() calls .rolling() with
only the window in both the single-window and the list-of-windows
branch. The value never reaches pandas, so rolling() keeps its default
of "a full window is required" and the leading rows stay NaN whatever
the user asks for.
On a 6-row frame with window=3:
min_periods=None -> [nan, nan, nan, 2.0, 3.0, 4.0]
min_periods=1 -> [nan, nan, nan, 2.0, 3.0, 4.0]
pandas reference -> [nan, 1.0, 1.5, 2.0, 3.0, 4.0]
ExpandingWindowFeatures already forwards it, so the two transformers
disagreed on a parameter they document identically.
The default is unchanged: min_periods=None is what rolling() already
assumed.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
WindowFeaturesdocumentsmin_periodsas a pandas passthrough:It is accepted and stored as
self.min_periods, buttransform()never reads it. Both branches build the window without it:So
rolling()keeps its default — a full window is required — and the leading rows stayNaNno matter what the user passes.ExpandingWindowFeaturesalready does forward it (.expanding(min_periods=self.min_periods)), so the two transformers disagreed on a parameter they document in the same words.Evidence
A 6-row frame,
window=3, defaultfunctions="mean",periods=1:min_periods=None[nan, nan, nan, 2.0, 3.0, 4.0]min_periods=1(before)[nan, nan, nan, 2.0, 3.0, 4.0]— identical, flag inertmin_periods=1(after)[nan, 1.0, 1.5, 2.0, 3.0, 4.0]X["x"].rolling(3, min_periods=1).mean().shift(1)[nan, 1.0, 1.5, 2.0, 3.0, 4.0]After the change the output matches pandas exactly, for a single window and for a list of windows.
Compatibility
The default is unchanged.
min_periods=Noneis precisely whatrolling()was already assuming, so anyone who never set the parameter sees identical output. Only users who explicitly passed it see a change — and it is the documented behaviour they asked for.Testing
Added
test_min_periods_is_usedandtest_min_periods_is_used_with_multiple_windows, comparing against pandasrolling(...)directly and asserting the default still requires a full window.tests/test_time_series: 121 passed