Repository navigation
fix: reject NaN holiday windows instead of silently dropping the range - #2761
Conversation
|
Hi @NishchayMahor! Thank you for your pull request and welcome to our community. Action RequiredIn 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. ProcessIn 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 If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
A NaN in
lower_windoworupper_windowpasses validation and then silently costs the holiday its entire window.Both rows carry
upper_window=1and both dates are inside the range, sopromo_delim_+1should fire on two days. The second holiday contributes only its day-zero flag. Itsupper_windowwas valid and was discarded anyway.Why it gets through
validate_inputsalready rejects a NaN indsorholiday, but the window columns are only range-checked:Both comparisons are
Falsefor NaN, so the row passes. Inmake_holiday_featuresit then reaches:int(nan)raisesValueError, the except sets both windows to 0, andrange(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
dsandholidayto 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 ValueErrorbranch. With this validation in place nothing reaches it through the constructor: NaN andNoneraiseValueErrorduring validation, and a non-numeric string raisesTypeErroron the existing.max() > 0comparison. 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_raisestoTestProphetHolidays, covering a NaN in each of the two columns.main:Failed: DID NOT RAISE ValueError.test_prophet.py: 62 passed, 2 skipped. Wholetests/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.pyandtest_prophet.pytoo, though a different part ofvalidate_inputs, so there may be a small textual conflict depending on merge order.