Fix tests that pass for the wrong reason, and export ZeroInflatedImputer - #217
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Reviewed with a fresh pass. The core claim reproduces: on @juaristi22's To do
Optional
Verified, no action
Merge orderThis carries #218's five-file docs reformat byte-identically, and it will conflict with #219, which rewrites the same |
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 b35dc4e: No implementation defect found in this PR. The remaining merge blocker is the inherited Lint failure. Merge blocker — incorporate #218 and rerun CI. The Lint job fails because Ruff would reformat five unchanged Markdown files: Validation at the reviewed head: 122 affected tests passed. Both Integration: read-only merge checks found no textual conflicts in the cumulative #218 + #214 + #215 + #216 + #217 tree. This is not a combined-suite result. #219 conflicts with #214, #215, #217, and #218, so its later rebase must retain the seed, Matching, API, and formatter fixes. The PR description cites an older final commit and historical green checks; update it to the resulting implementation and current validation. |
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.
b35dc4e to
94fb136
Compare
Critical fixedUpdated the branch onto current Should-address fixedRefreshed the description to reflect dependency-aware test selection, both public Verification
Pushed head: GitHub CI: All seven current-head checks passed, including lint, changelog, Python 3.12 smoke tests, Python 3.14 tests with R, and Vercel preview. GitHub reports this pull request as mergeable. (run). |
This branch was successfully deployed
2 participants
Fixes #204 and addresses the test and export items from #213.
The autoimpute tests use its dependency-aware default model selection, so environments without optional R or MDN dependencies exercise the available models. Error tests check the expected messages, and the determinism test rejects missing losses before comparing results.
ZeroInflatedImputeris available from bothmicroimputeandmicroimpute.models.VALID_YEARSandDEFAULT_MODEL_PARAMSremain importable with their existing values for downstream compatibility. Comments distinguish these legacy values from runtime model defaults, and tests check the supported-year list and parameter mapping's public shape.The fit docstring now describes native sample weights and identifies QuantReg and MDN as raising
NotImplementedErrorfor weighted fits.Validation: 122 affected tests passed before the base rebase, and both public
ZeroInflatedImputeraliases resolved to the implementation class. Optional R/MDN dependencies were unavailable locally. Verification confirmed unchanged program and test contents after the rebase. The branch includes #218's shared lint repair; Ruff 0.16.7 formatting andgit diff --checkpass.Checks for current head
94fb136.