Fix pre-submission imputation and evaluation correctness - #219
juaristi22 wants to merge 25 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Reviewed this in depth against
Zero-inflated quantiles: 0 non-monotone entries across 13 quantiles × 300 rows, zero atom respected. QuantReg single-row constant-predictor prediction 6.9959 against a true 7. Suite: Requesting changes on five blocking items. Blocking
Merge-order note#214 must land before this rebases, because this PR regresses it. Its Non-blocking
Changelog fragment types are right — the four |
|
Two additions to the review above. First, credit where it is due: the coverage figures in the description reproduce digit-for-digit. I replicated the fixture from Identical to the quoted numbers. On an independent fixture (normal predictors, 3000/2000) the same improvement holds — 18.20/49.80/80.80 → 9.95/50.05/88.50 — so it is not fixture-shopped. Second, a concrete split, since "split the PR" is easy to say and annoying to receive without a proposal. Each step is independently revertable, in merge order:
If a full rebase is too costly, the minimum useful version is pulling Matching out — it is the largest single file change, and the only part with no verification path outside CI. Two more pieces of evidence for items already listed. On integer targets, a Poisson(1.2) fixture gives |
The previous approach reformatted five documentation files and bounded ruff to <0.17.0. Per review, the bound was a no-op against the failure it targeted: 0.16.0 through 0.16.8 all format markdown and all satisfy it, so only the reformatting was doing any work - and the next formatter change inside 0.16.x would reopen the failure. Excludes docs/**/*.md via [tool.ruff] instead, which fixes the cause rather than the symptom, and drops the five reformatted files. That also removes the byte-identical docs hunks that collided with #216, #217 and #219. The dev extra is bounded to match CI, so a contributor running make format no longer reformats files CI then rejects. The cause was ruff 0.16.0, not 0.16.7: 0.13, 0.14 and 0.15 report '65 files already formatted' and do not scan markdown at all.
|
Review of b2c7ce3: changes required. Three defects reproduced through supported public APIs, and optional MDN validation remains incomplete.
Validation gap: the current filename-based MDN gate only enables the extra when a changed path contains Integration: merge #218 first and bring #214–#217 to green CI before integrating them. Exact-head merge checks found conflicts with #214 ( Validation at the reviewed head: the existing local suite passed 469 tests, with three optional runtime skips. The focused seed, mixture-seed, constant-probability, autoimpute/replay, and refit reproductions above exposed gaps beyond that suite. All eight scheduled remote checks passed, but the MDN gate means those results do not establish MDN compatibility. No combined-suite pass is claimed. The existing review and follow-up contain additional feedback that is being investigated separately. The three defects above are the findings reproduced in this review. |
Every per-variable model builds its own generator from the seed it is given, and all of them were handed self.seed. They therefore drew the same random quantiles in the same row order, so variables imputed together came out rank-comonotonic whatever their dependence in the donor: three targets with nil conditional dependence reproduced at 0.71 Spearman, 0.11 after this change. The offset follows the convention already used for the subsampling seed in _apply_max_train_samples. QRF also now accepts a seed argument; there was previously no way for a caller to vary the draws. Fixes #207
Per review, two ways the fix could be undone silently. _seed_for_variable fell back to offset 0 when a variable was not in imputed_variables, which hands it the same draws as the first target - the comonotonicity this method exists to prevent, reintroduced with no symptom. It now raises. Today's call sites all pass post-preprocessing names, so this is about the next refactor, not current behaviour. Seeds were validated lazily inside _seed_for_variable, so QRF(seed=-5) constructed fine and only failed part-way through fit, where the blanket handler rewrapped the ValueError as RuntimeError. Validation now happens in __init__, bools included, and the test asserts ValueError at construction rather than RuntimeError at fit. Also drops the docs reformatting, which belongs to #218 alone.
Both tuning handlers caught every exception and substituted the training mean, with no log and no counter, and that score went straight into the Optuna objective. A mean-predictor is not a neutral score - on a low-signal target it can beat a genuine matching fit on quantile loss - so a parameter set under which matching always failed could be selected as best and reported as the winning method. Both now log the exception and raise TrialPruned. The predict path keeps its NaN fill, which is the right behaviour there, but now reports the total number of unmatched records rather than leaving silent NaN blocks. Fixes #210
Per review, three follow-ups. n_failed_records kept the previous successful call's value when a prediction raised before reaching _process_matching_results, so a caller reading it after an exception got a stale number. It is now reset on entry to _predict, and documented - including that Matching runs single-threaded, so the attribute is safe in practice but would race under concurrent calls on one fitted object. The count is also mirrored onto result.attrs['n_failed_records'], so a caller does not have to reach into the fitted model for it. An all-pruned study reported only that nothing succeeded. It now carries the most recent underlying failure, which is what a user needs when matching fails structurally - a bad dtype or a missing R package - and the cause was otherwise reachable only through __cause__. Also drops the docs reformatting, which belongs to #218.
The previous commit de-indented 'raise optuna.TrialPruned() from e' out of 'except Exception as e', so 'e' was unbound and the trial failed with UnboundLocalError instead of pruning - which the matching failure tests caught.
…ing failure metadata
packages = ["microimpute"] with package-data "**/*" meant the subpackages reached the wheel only as package data, swept in by a glob that also collected whatever else was in the working tree. A wheel built from a tree with compiled bytecode present carried 27 __pycache__ entries and around 500 KB of build-host bytecode, so wheel contents were a function of the builder's working directory rather than the source. setuptools.packages.find declares them properly. Verified: with 54 .pyc files present in the tree, the built wheel now contains none, and every subpackage imports from the installed wheel. Also adds py.typed, so the annotations become visible to downstream consumers; the two missing authors, which left the paper's corresponding author out of the PyPI metadata; and classifiers and project URLs, with no repository link previously on the PyPI page. The absent LICENSE file is #197 and is not addressed here. Fixes #211
Per review. Adds Python 3.12/3.13/3.14 classifiers to match requires-python, an email for Vahid so all four authors render in Author-email rather than one splitting into the legacy Author field, and an explicit exclude alongside the include as belt and braces. Rewords the changelog fragment: the bloat is latent rather than shipped. CI builds from a fresh checkout with no imports, so published wheels have been clean - the problem appears when anyone builds from a tree that has been tested in. The previous wording implied released wheels were affected. Also drops the docs reformatting, which belongs to #218.
tests/test_autoimpute.py named Matching unconditionally whenever MDN was absent, though HAS_MATCHING was already computed and unused. Without rpy2 that raised NameError before the call under test ran, which is the whole of #204: eight errors that looked like failures of the code under test. An available_models() helper builds the list from what actually imported. That NameError was also masking a real problem. Four pytest.raises calls had no match=, so they passed on any exception - including that NameError. test_autoimpute_missing_predictors was passing on it rather than on the missing-column error it claims to test, which is visible now that each raises names the message it expects. A fifth test asserted inside an except branch, so it would have passed silently if predict ever stopped raising, and a sixth skipped its assertion when both losses were NaN, which is exactly the case #210 produces. ZeroInflatedImputer was reachable only by full module path despite being a documented feature of the paper. DEFAULT_MODEL_PARAMS and VALID_YEARS had no references anywhere in the package, and the Imputer.fit docstring still described the bootstrap resampling scheme that was replaced by native sample_weight support. Fixes #204
Matching surfaces a missing predictor from R rather than from pandas, so the message differs from the other models'. Only visible where rpy2 and StatMatch are installed.
The DEFAULT_MODEL_PARAMS test asserted the whole mapping as a literal against itself, which froze values nothing in the package reads - it has no callers inside microimpute and the real defaults live in each model. It now checks the keys and shapes, which is what a downstream caller relies on. The constant itself stays, per @juaristi22's compatibility fix; VALID_YEARS keeps its exact assertion because two notebooks depend on those years. Drops test_published_notebook_config_imports: it parsed an 8 MB notebook to assert what the two tests above it already assert, and would fail as a confusing KeyError if either notebook were renamed. available_models() now returns None. autoimpute's own default is already dependency-aware, so the helper was duplicating production logic and, as written, only exercised Matching and MDN when MDN happened to be installed. The Imputer.fit weight docstring said weights go to the learner's weighted-fit interface without noting that QuantReg and MDN raise NotImplementedError. The paper in #201 makes claims about exactly this. Also drops the docs reformatting, which belongs to #218.
Every per-variable model builds its own generator from the seed it is given, and all of them were handed self.seed. They therefore drew the same random quantiles in the same row order, so variables imputed together came out rank-comonotonic whatever their dependence in the donor: three targets with nil conditional dependence reproduced at 0.71 Spearman, 0.11 after this change. The offset follows the convention already used for the subsampling seed in _apply_max_train_samples. QRF also now accepts a seed argument; there was previously no way for a caller to vary the draws. Fixes #207
Both tuning handlers caught every exception and substituted the training mean, with no log and no counter, and that score went straight into the Optuna objective. A mean-predictor is not a neutral score - on a low-signal target it can beat a genuine matching fit on quantile loss - so a parameter set under which matching always failed could be selected as best and reported as the winning method. Both now log the exception and raise TrialPruned. The predict path keeps its NaN fill, which is the right behaviour there, but now reports the total number of unmatched records rather than leaving silent NaN blocks. Fixes #210
b2c7ce3 to
5dfc865
Compare
|
Re-checked the five blockers against The pickle blocker is only half fixed, and QRF still breaks. Your Reproduction on this head — fit, pickle, strip the new attributes to simulate a model pickled by an earlier release, predict: OLS is fixed. QRF fails at if (
quantiles is not None
and self.sequential # <- unguarded
and len(self.imputed_variables) > 1
):
This matters because Two lines would do it: I have not pushed anything to this branch — you are clearly mid-flight on it, and I did not want to collide. Happy to push the two-line guard plus a round-trip regression test if that is useful. The remaining blocker from my earlier review is the split, which is your call rather than a defect. Now that #214, #215, #216, #217 and #218 are all on |
This branch was successfully deployed
2 participants
The pre-submission audit in #201 identified silent errors in preprocessing, conditional distributions, sampling, and method comparison. This change preserves donor-fitted transformations, computes probability-based scores, and prevents unsupported distribution forecasts from entering model selection.
Correctness changes
train_sizeand model seeds; select final tuned parameters through fresh training-data tuning after fold scoring.Fixes #202, fixes #203, fixes #204, fixes #205, fixes #206, fixes #208, fixes #209, fixes #212. Addresses the validation portion of #213.
This branch includes the existing fixes from #214 (#207), #215 (#210), and #218 (pre-existing lint failures), retaining their authorship. Those PRs can merge separately before rebasing this branch; this does not replace the JOSS paper PR.
Compatibility and publication
Numeric-coded categorical targets now require a categorical dtype or
target_types={"variable": "categorical"}. UseQRF(sequential=False)for marginal quantiles of multiple targets. Quantiles of a sequential multivariate chain require a different estimator and now fail explicitly. Zero-inflated models offer independent mode for the same reason.These corrections change imputed values and model rankings. Regenerate paper benchmarks and affected downstream datasets before submission. QRF retaining all leaf samples can increase memory use. This PR does not resolve the separate license, authorship, metadata, release/DOI, and documentation requirements tracked by #201.
Adversarial review fixes
The follow-up review reproduced five edge-case failures; all five are repaired with regression tests:
quantiles=Noneand attach exact point-mass probabilities using the documented result container.Validation
All scheduled checks passed on repair commit
b2c7ce3d8336669bc1ee1c3dfbb3a75569c6db67. The Python 3.14 CI run completed with 484 passed, 2 skipped in 239.53 seconds, including all 33 new regression cases and all 15 real-R Matching tests. The end-to-end pipeline, Python 3.12 smoke checks, lint, changelog, documentation build, and Vercel checks also passed. Formatting passed for 113 files; the post-format affected test run passed 118 tests.The new regression suite reproduced 25 failures with 8 passing controls before repair. All 33 new cases now pass. Independent verification passed 31 checks and found all five planned repairs complete. The affected suite passed 425 tests, 1 skipped; the complete local Python 3.13.14 suite passed 469 tests, 3 skipped. The skips are the optional R and MDN runtime modules.
On a controlled normal-noise QRF fixture (3,000 training and 2,000 test observations), nominal q10/q50/q90 coverage changed from 27.45%/48.8%/70.25% to 11.15%/50.9%/88.3%. This is a regression fixture, not a universal calibration guarantee.
Optional MDN validation remains a gap because the existing workflow skips its dependency installation for this diff. This PR remains a draft; the separate submission requirements above are still open.
The CI suite reported 207 warnings, chiefly rpy2 deprecations plus solver/dependency warnings. Coverage XML was generated, but Codecov rejected the upload because a protected branch requires a token. The workflow treats that upload failure as non-blocking.