fix(examples): compute the discrete-label threshold from the training split only - #1584
Open
Amir Fathi (AmirF194) wants to merge 3 commits into
Open
Conversation
… split only `above_mean_sales` in the "Forecasting Discrete Values" example was thresholded on `df["Sales"].mean()` over the full 180-day series, before the train/test split. That mean includes the 30-day held-out test period, so the training labels are partly derived from data the model is supposed to be evaluated against later, a train/test leak. Fix: split into `discrete_train_df` / `discrete_test_df` first, compute the mean from `discrete_train_df["Sales"]` only, and threshold both frames with that single training-derived value. Applied to both the notebook and its mirrored markdown doc. Fixes microsoft#1175
Author
|
@microsoft-github-policy-service agree |
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents test-period data leakage in the discrete forecasting example.
Changes:
- Splits data before computing the label threshold.
- Uses the training-period mean for both training and test labels.
- Mirrors the fix in the documentation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
notebook/automl_time_series_forecast.ipynb |
Corrects threshold computation in the notebook. |
website/docs/Examples/AutoML-Time series forecast.md |
Mirrors the corrected example logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| # feature engineering - create a discrete value column | ||
| # 1 denotes above mean and 0 denotes below mean, thresholded on the | ||
| # training period's mean (no leak into discrete_test_df) |
Comment on lines
+2080
to
+2081
| "discrete_train_df[\"above_mean_sales\"] = np.where(discrete_train_df[\"Sales\"] > train_mean_sales, 1, 0)\n", | ||
| "discrete_test_df[\"above_mean_sales\"] = np.where(discrete_test_df[\"Sales\"] > train_mean_sales, 1, 0)\n", |
| "discrete_test_df = df[-time_horizon:].copy()\n", | ||
| "# feature engineering - create a discrete value column\n", | ||
| "# 1 denotes above mean and 0 denotes below mean, thresholded on the\n", | ||
| "# training period's mean (no leak into discrete_test_df)\n", |
Author
|
No rush, just checking in since it's been about a week. Happy to rebase or adjust scope if that would help get it reviewed. |
Li Jiang (thinkall)
requested changes
Sep 2, 2026
Li Jiang (thinkall)
left a comment
Li Jiang (thinkall)
left a comment
Contributor
There was a problem hiding this comment.
Two issues remain before merging:
- The corrected threshold changes the training labels, but the notebook still contains downstream outputs generated with the old labels, including the model/predictions and
accuracy = 1.0. Please rerun cells 23–30 or clear those outputs. - The comment “No leak into
discrete_test_df” describes the leakage direction incorrectly. The fix prevents test-period sales from influencing the training labels; the training-derived threshold is intentionally applied to the test frame. Please correct this wording in both the notebook and markdown mirror.
…utputs thinkall's review on the fix: the comment said "no leak into discrete_test_df", but the fix guards the threshold itself, not the test frame (applying the training mean to the test frame is intentional). Reworded in both the notebook and the markdown mirror. Also cleared the notebook's downstream outputs (train_df preview, fit, best config, predictions, accuracy) since they were generated against the old, leaky labels and no longer match the code.
Author
|
Both addressed. Cleared the outputs on the fit/predict/accuracy cells rather than rerunning them, since I didn't have hcrystalball installed in this environment and didn't want to commit numbers from a run you couldn't check against. Reworded the comment: the threshold is what stays training-only, the test frame reusing that same mean is intentional, not something the fix guards against. |
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.
Why are these changes needed?
In the "Forecasting Discrete Values" example,
above_mean_salesis thresholded ondf["Sales"].mean()computed over the full 180-day series, before the train/test split happens a few lines later. That mean is derived in part from the 30-day period held out asdiscrete_test_df, so the training labels carry information from the data the model is later evaluated against.This PR splits into
discrete_train_df/discrete_test_dffirst, computes the threshold fromdiscrete_train_df["Sales"]only, and applies that single value to both frames. Same fix applied to both the notebook and its mirrored markdown doc underwebsite/docs/Examples/.Verification:
hcrystalball.utils.get_sales_data, same call the cell uses): the full-series mean (17020.14) differs from the train-only mean (16833.40) and changes at least one training row's label.discrete_X_train/discrete_X_testkeep their original shapes (150/30 rows) anddiscrete_train_dfkeeps its original column set.AutoML.fit/predictcells (no stored outputs exist for the cell I changed, and no CI test executes this notebook), so I can't speak to any effect on the reported accuracy number further down, only that the label computation itself no longer touches the test period.website/docs/Examples/AutoML-Time series forecast.md) for the same pattern and fixed it identically; ranpre-commiton both changed files (mdformatreflowed two lines in the doc, otherwise clean).Related issue number
Fixes #1175
Checks
automl_time_series_forecast.ipynb; the fix was verified by running the cell's logic standalone, see above).