Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
853ffdf to
c58c9e3
Compare
061fa2e to
1e8d8ce
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## pymc6_and_pymcmarketing1_migration #1113 +/- ##
===================================================================
Coverage 97.14% 97.15%
===================================================================
Files 136 136
Lines 24586 24672 +86
Branches 1403 1407 +4
===================================================================
+ Hits 23885 23970 +85
Misses 484 484
- Partials 217 218 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7b0298e to
f82cde5
Compare
c58c9e3 to
d158d16
Compare
29a1d0c to
d1c2cec
Compare
b9da23f to
031c83d
Compare
d1c2cec to
27c2dce
Compare
031c83d to
e13d969
Compare
27c2dce to
29c2147
Compare
e13d969 to
e855435
Compare
29c2147 to
d598dd7
Compare
d598dd7 to
79b97a1
Compare
29d9a10 to
bdff957
Compare
aee603b to
7d3502c
Compare
…stacklevel get_inclusion_probabilities() and get_shrinkage_factors() returned a bare RangeIndex, so the caller had to know the fit-time column order to read them. That is the wrong contract for a feature whose output is a claim about which covariates matter: the correctness test sliced positionally and the notebook in #1113 relabelled the index by hand. Rows are now indexed by the regressor names the model already tracks in _exog_names. The beta_exog precedence warning goes to stacklevel 3. pm.Model's metaclass calls __init__, so at 2 the warning was attributed to pymc/model/core.py instead of the line that passed both arguments. The vs_hyperparams docstring said the horseshoe scales its global shrinkage from the data. Only the sample size comes from the data; the residual scale in the Piironen and Vehtari rule is held at 1.
9ee29e1 to
91640f4
Compare
|
indeed |
|
@anevolbap I don't recognize the branch, can we point to migration branch instead? |
…stacklevel get_inclusion_probabilities() and get_shrinkage_factors() returned a bare RangeIndex, so the caller had to know the fit-time column order to read them. That is the wrong contract for a feature whose output is a claim about which covariates matter: the correctness test sliced positionally and the notebook in #1113 relabelled the index by hand. Rows are now indexed by the regressor names the model already tracks in _exog_names. The beta_exog precedence warning goes to stacklevel 3. pm.Model's metaclass calls __init__, so at 2 the warning was attributed to pymc/model/core.py instead of the line that passed both arguments. The vs_hyperparams docstring said the horseshoe scales its global shrinkage from the data. Only the sample size comes from the data; the residual scale in the Piironen and Vehtari rule is held at 1.
a897cec to
cce4b29
Compare
Remove the experimental FutureWarning from StateSpaceTimeSeries: the API is now aligned with the other PyMCModel subclasses, covariates and variable selection are supported, and edge cases are guarded and tested. This commit also pointed the BayesianBasisExpansionTimeSeries warning at StateSpaceTimeSeries as a deprecation. Review on #1113 asked to keep that model, so a later commit in this series restores it to its experimental state. Record the change in the release notes under the 1.0.0 breaking-change section. Completes the P0 graduation decision for #758.
18f6f7f to
d02a04e
Compare
…precating it Review feedback on #1113: the model should not be deprecated. Restore its experimental FutureWarning and docstring to the base state, and update ARCHITECTURE.md, the ITS skill reference and the release notes. StateSpaceTimeSeries still graduates.
|
Dropped the deprecation: Also rebased onto the current #1112 head, so the conflict is gone, and the notebook now calls |
|
@anevolbap if we are ready then let me know moving this to ready for review, so I can merge. |
|
Moving this to the 1.1.0 milestone. The migration branch is being frozen so the PyMC 5 vs 6 baseline for #1048 can be pinned to the exact tree that goes to |
Reject seasonal_length below 2 (previously an obscure ZeroDivisionError inside pymc-extras) and warn when out-of-sample dates do not continue the training frequency (forecast values map onto X's dates by position). Lock missing-value support with a test: the Kalman filter handles NaN in y natively and predictions stay finite, which covers the P2 item of #758 for this model.
Remove the experimental FutureWarning from StateSpaceTimeSeries: the API is now aligned with the other PyMCModel subclasses, covariates and variable selection are supported, and edge cases are guarded and tested. This commit also pointed the BayesianBasisExpansionTimeSeries warning at StateSpaceTimeSeries as a deprecation. Review on #1113 asked to keep that model, so a later commit in this series restores it to its experimental state. Record the change in the release notes under the 1.0.0 breaking-change section. Completes the P0 graduation decision for #758.
New docs/source/notebooks/interrupted-time-series-bsts.ipynb covers the graduated StateSpaceTimeSeries with InterruptedTimeSeries: trend and seasonality only, exogenous control covariates, and spike-and-slab covariate selection with inclusion probabilities. Executed end to end. Cites the existing Brodersen et al. (2015) entry, adds the gallery card and the ITS toctree entry, updates the ARCHITECTURE.md model list, and documents the smoothed-posterior R2 caveat on score(). Part of #696 (the notebook, not yet the guidance on when BSTS is the better choice) and the documentation item of #758 P0.
level_order below 1 reached pymc-extras and failed there with 'index -1 is out of bounds' or 'negative dimensions are not allowed', the same failure mode the seasonal_length guard already covered. The seasonal_length message offered a model without seasonality via a custom seasonality_component. There is no such path: build_model always adds a seasonal component, and pymc-extras ships no identity component.
… claim mock_pymc_sample is session-scoped, so once any earlier test requests it pm.sample stays patched. The three tests added here without it were therefore mocked in any real run: test_missing_y_values_handled asserted that a posterior is finite against prior draws. It is now correctness-marked so it runs against real NUTS. The other two assert shape and a warning, so they request the fixture explicitly, which is what the rest of the file already does.
…able Rebuilding a DatetimeIndex from raw values drops its freq, so every fit emitted a pymc-extras 'No frequency was specific on the data's DateTimeIndex' warning and the forecast index had to re-infer it. Recover it when the observations are regularly spaced, and leave it unset when they are not. combined.build() also printed a rich 'Model Requirements' table to stdout on every construction, telling the reader to declare priors that this class declares itself a few lines later. The docstring example in #1112 had to wrap the call in redirect_stdout to hide it.
The thumbnail was force-added past .gitignore:18, which ignores that directory because docs/source/conf.py regenerates it from the notebook on every Sphinx build. It was the only tracked file there. The skills reference still called both time-series models experimental. The release note claimed the graduation put the class under the Tier 2 promise, but ARCHITECTURE.md defines Tier 2 mechanically and pymc_models is already listed in docs/source/api/index.md, so the tier did not change. Replaced with the two warnings that did stop firing.
Re-run against the current stack, so the stored outputs no longer carry the pymc-extras requirements table or the frequency warning. That also drops three absolute site-packages paths from the stderr output. Three prose fixes. The plot title reports a Bayesian R2 of 1 with a standard deviation around 1e-9, because the Kalman smoother conditions on the observed outcome and interpolates the pre-period; the text now says so instead of leaving the reader with an apparently perfect fit. The two divergences in the covariates fit are acknowledged. The rhat paragraph told the reader to fall back on the inclusion probabilities, which are averaged over the very chains that disagree, so it now calls the ranking provisional for that reason. The hand-written index on the inclusion chart is gone: the labels come from the model since #1112.
…precating it Review feedback on #1113: the model should not be deprecated. Restore its experimental FutureWarning and docstring to the base state, and update ARCHITECTURE.md, the ITS skill reference and the release notes. StateSpaceTimeSeries still graduates.
The PR is in the 1.1.0 milestone, so its note leaves the 1.0.0rc1 draft and starts a 1.1.0 draft in the same format.
d02a04e to
bc57466
Compare
Part of #982, of #758 (P0) and of #696. Milestone 1.1.0. Targets
pymc6_and_pymcmarketing1_migrationfor now and lands onmainafter 1.0.0rc1; rebased on the current migration tip after #1112 merged, so the diff is this PR's 11 commits only.FutureWarningfromStateSpaceTimeSeries.BayesianBasisExpansionTimeSeriesis unchanged. It stays experimental and keeps itsFutureWarning, per review. A test pins that.level_orderbelow 1,seasonal_lengthbelow 2, missing values iny(the Kalman filter imputes them natively, which covers the P2 item of Feature parity with Google's CausalImpact #758 for this model), integer-index error path, and a warning when out-of-sample dates do not continue the training frequency. The missing-value test iscorrectness-marked so it runs against real NUTS: asserting that a posterior is finite says nothing against the suite's mockedpm.sample.verbose=False, so construction no longer prints the pymc-extras "Model Requirements" table, which tells the reader to declare priors this class declares itself.interrupted-time-series-bsts.ipynb: trend and seasonality, then covariates, then spike-and-slab with inclusion probabilities. Added togallery.yaml.score()..github/release-drafts/1.1.0.md, in the format of the rc1 draft, covering the removed warning and the newseasonal_lengthandlevel_orderValueErrors. The rc1 draft is untouched since this PR is not in that release.Three items from #982 are deliberately not here, since each is a separate behavior change worth its own review: making
StateSpaceTimeSeriesthe default model forInterruptedTimeSeries, exporting it at top level, and adding acp.InterruptedTimeSeries(data, treatment_time)convenience entry point. The first depends on data-scaled priors (#935). Thenp.ndarraysignature item listed in #982 is already stale: both time-series models takexr.DataArraytoday, andInterruptedTimeSeriesdispatches onPyMCModelagainstRegressorMixin, not on the model class.One gap worth naming: #758 and #982 both list "no seasonality" as an edge case to test, and no such path exists.
build_modelalways adds a seasonal component, andpymc-extrasships no identity component, so theseasonal_lengthguard now says that plainly instead of pointing at aseasonality_componentworkaround that does not produce a seasonality-free model. A real trend-only option is a separate change if you want it.Note for reviewers:
score()is computed on smoothed in-sample predictions, so it reads optimistic relative to other models. Documented, not changed here.On the notebook: it is honest about what the demo shows rather than overselling it. With 30 post-period days and diffuse default priors, the trend-only model does not identify the effect (95% interval includes zero, posterior probability of an increase around 0.75). Adding
x1andx2tightens the interval substantially and raises that probability to about 0.87, but the effect is still not credibly non-zero at the 95% level. The plot titles report a Bayesian R² of 1 on the pre-period; the notebook explains that this comes from the Kalman smoother conditioning on the observed outcome and is not a measure of fit. Thebeta_exogposterior means sit on the true 2.0 and -1.5 while the individual 94% HDIs span zero, because the state-space level and the regressors compete for the same variation. In the selection example the two real predictors take the top two inclusion places but nothing clears 0.5, so the notebook tells the reader to use the ordering and not the level. If you would rather the notebook demonstrate a clean detection, the setup needs a change (shorter horizon,level_order=1, or data-scaled priors) and I will make it.Verified locally with pymc 6.2.0, arviz 1.2.0, pymc-extras 0.14.0 on the rebased head: 2592 passed, 20 skipped, 6 deselected; the correctness lane 5 passed;
diff-coveragainstpymc6_and_pymcmarketing1_migrationcovers 100% of the 10 changed executable lines;mypyclean;prek run --all-filesclean; doctests forpymc_models.py8 passed. The notebook calls.fit()on its three experiments and runs end to end under the mock runner (runner.py --notebook, 16 cells). Its stored outputs come from the earlier full run (runner.py --full, no mocks) and were not regenerated. The selection model reports rhat above 1.01, which is inherent to a spike-and-slab posterior rather than a sampling setup problem, so the notebook says so and points the reader at the inclusion probabilities; the covariate model shows 2 divergences. Sampling is not bit-reproducible across runs even withrandom_seedset, so the notebook prose avoids quoting values that drift.