Skip to content

Pass min_periods to rolling() in WindowFeatures - #1043

Open
VenishPaneliya wants to merge 1 commit into
feature-engine:mainfrom
VenishPaneliya:window-features-min-periods
Open

Pass min_periods to rolling() in WindowFeatures#1043
VenishPaneliya wants to merge 1 commit into
feature-engine:mainfrom
VenishPaneliya:window-features-min-periods

Conversation

@VenishPaneliya

Copy link
Copy Markdown
Contributor

What this fixes

WindowFeatures documents min_periods as a pandas passthrough:

min_periods: int, default None. Minimum number of observations in the window required to have a value; otherwise, the result is np.nan. See parameter min_periods in pandas rolling() documentation for more details.

It is accepted and stored as self.min_periods, but transform() never reads it. Both branches build the window without it:

X[self.variables_].rolling(window=win)          # list-of-windows branch
X[self.variables_].rolling(window=self.window)  # single-window branch

So rolling() keeps its default — a full window is required — and the leading rows stay NaN no matter what the user passes.

ExpandingWindowFeatures already 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, default functions="mean", periods=1:

result
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 inert
min_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=None is precisely what rolling() 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_used and test_min_periods_is_used_with_multiple_windows, comparing against pandas rolling(...) directly and asserting the default still requires a full window.

  • without the fix: both fail
  • with the fix: both pass
  • tests/test_time_series: 121 passed
  • flake8 and black clean on both files

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant