Fix utilsforecast evaluation compatibility #282
Conversation
Handle missing cutoff/id_col in evaluation output Make cross-validation robust to utilsforecast schema changes
Add cutoff_col to mase and forward it to mae for correct evaluation
AzulGarza
left a comment
AzulGarza
left a comment
There was a problem hiding this comment.
thanks @Kushagra7777! since the pr introduced the new cutoff_col, we need to update the tests as well.
AzulGarza
left a comment
AzulGarza
left a comment
There was a problem hiding this comment.
thanks, @Kushagra7777! it seems the tests are working now, one remaining thing is fixing lint and styling checks.
|
i'm also adding copilot as reviewer to have its comments. |
There was a problem hiding this comment.
Pull request overview
This pull request enhances the robustness of the experiment handler to handle schema changes in the utilsforecast library. The changes make the evaluation and cross-validation functionality more resilient when the utilsforecast library's output schema varies (e.g., missing cutoff or id_col columns).
Key changes:
- Introduced
_zero_to_nan_pdhelper function to replace reliance on utilsforecast's_zero_to_nan - Updated the
masefunction to defensively handle optional cutoff columns in evaluation output - Added defensive column filtering in
evaluate_forecast_dfto handle missing cutoff/id_cutoff columns
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if "cutoff" not in eval_df.columns: | ||
| if "id_cutoff" in eval_df.columns: | ||
| eval_df = eval_df.merge(cutoffs, on="id_cutoff", how="left") | ||
| else: | ||
| pass | ||
|
|
||
| cols = ["unique_id", "cutoff", "metric"] + models | ||
| cols = [c for c in cols if c in eval_df.columns] | ||
| eval_df = eval_df[cols] |
There was a problem hiding this comment.
The new defensive logic for handling missing cutoff/id_cutoff columns (lines 271-275) and the column filtering (lines 277-279) lack test coverage. Consider adding test cases that verify the behavior when utilsforecast's evaluate() function returns DataFrames with different schema variations (e.g., missing cutoff column, missing id_cutoff column, or both).
| def _zero_to_nan_pd(s: pd.Series) -> pd.Series: | ||
| s = s.astype(float).copy() | ||
| s[s == 0] = np.nan | ||
| return s |
There was a problem hiding this comment.
The new _zero_to_nan_pd helper function lacks direct test coverage. While it may be indirectly tested through the mase function, consider adding a direct unit test to verify its behavior, especially for edge cases like empty series, all-zero series, and series with mixed zero and non-zero values.
| seasonality: int, | ||
| train_df: pd.DataFrame, | ||
| id_col: str = "unique_id", | ||
| time_col: str = "ds", |
There was a problem hiding this comment.
The time_col parameter is added to the function signature but is never used in the function body. Consider removing this parameter if it's not needed, or if it was added for future use, document its intended purpose.
Update live tests to query using the actual unique_id from generated data instead of a hardcoded value. This avoids empty series selection and prevents flaky failures when checking queryability.
as suggested by copilot Co-authored-by: Copilot <[email protected]>
as suggested by copilot Co-authored-by: Copilot <[email protected]>
Handle missing cutoff/id_col in evaluation output
Make cross-validation robust to utilsforecast schema changes