Skip to content

fix: reject NaN holiday windows instead of silently dropping the range - #2761

Merged
tcuongd merged 1 commit into
facebook:mainfrom
NishchayMahor:fix/nan-holiday-window
Oct 22, 2026
Merged

tcuongd merged 1 commit into
facebook:mainfrom
NishchayMahor:fix/nan-holiday-window

Conversation

@NishchayMahor

Copy link
Copy Markdown
Contributor

A NaN in lower_window or upper_window passes validation and then silently costs the holiday its entire window.

hol = pd.DataFrame({
    "holiday": ["promo", "promo"],
    "ds": pd.to_datetime(["2026-01-15", "2026-02-15"]),
    "lower_window": [-2, np.nan],     # one window is missing
    "upper_window": [1, 1],
})
m = Prophet(holidays=hol)             # no error
feats, _, _ = m.make_holiday_features(
    pd.Series(pd.date_range("2026-01-01", periods=90, freq="D")), m.holidays
)
print(feats.sum().astype(int))
promo_delim_+0    2
promo_delim_+1    1
promo_delim_-1    1
promo_delim_-2    1

Both rows carry upper_window=1 and both dates are inside the range, so promo_delim_+1 should fire on two days. The second holiday contributes only its day-zero flag. Its upper_window was valid and was discarded anyway.

Why it gets through

validate_inputs already rejects a NaN in ds or holiday, but the window columns are only range-checked:

if self.holidays['lower_window'].max() > 0:
    raise ValueError('Holiday lower_window should be <= 0')
if self.holidays['upper_window'].min() < 0:
    raise ValueError('Holiday upper_window should be >= 0')

Both comparisons are False for NaN, so the row passes. In make_holiday_features it then reaches:

try:
    lw = int(getattr(row, 'lower_window', 0))
    uw = int(getattr(row, 'upper_window', 0))
except ValueError:
    lw = 0
    uw = 0

int(nan) raises ValueError, the except sets both windows to 0, and range(0, 1) yields only offset 0. Nothing is logged.

The realistic way to hit this is building the holidays frame from a merge or a CSV with a blank cell, which is also the case where you are least likely to notice: the model fits, predicts, and is quietly missing part of the holiday specification you gave it.

Fix

Extend the NaN check that already exists for ds and holiday to cover the window columns, so it is caught at the boundary with the same error the frame already raises elsewhere.

I deliberately did not touch the except ValueError branch. With this validation in place nothing reaches it through the constructor: NaN and None raise ValueError during validation, and a non-numeric string raises TypeError on the existing .max() > 0 comparison. Fixing the cross-contamination there would have been unreachable code, so it seemed better left alone. Worth knowing it is latent if that validation is ever relaxed.

Testing

Added test_holidays_nan_window_raises to TestProphetHolidays, covering a NaN in each of the two columns.

  • On main: Failed: DID NOT RAISE ValueError.
  • With the change: passes.
  • test_prophet.py: 62 passed, 2 skipped. Whole tests/ directory: 122 passed, 2 skipped.

Not verified: I ran the suite without a compiled CmdStan, so the tests that actually fit a model are not among those exercised here. The change is in validation and is reached before any backend call.

Heads-up: #2757 touches forecaster.py and test_prophet.py too, though a different part of validate_inputs, so there may be a small textual conflict depending on merge order.

@meta-cla

meta-cla Bot commented Oct 21, 2026

Copy link
Copy Markdown

Hi @NishchayMahor!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@meta-cla meta-cla Bot added the CLA Signed label Oct 22, 2026
@meta-cla

meta-cla Bot commented Oct 22, 2026

Copy link
Copy Markdown

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@tcuongd
tcuongd merged commit 5cf15c0 into facebook:main Oct 22, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants