Fix the Lint job, which is failing on main by vahid-ahmadi · Pull Request #218 · PolicyEngine/microimpute

vahid-ahmadi · 2026-09-16T12:13:39Z

Ruff 0.16.0 began formatting Python code blocks in Markdown, causing ruff format --check . to fail on five documentation files in an untouched checkout of main. This PR excludes docs/**/*.md through Ruff’s extend-exclude setting and aligns the CI installation and development extra on ruff>=0.16.0,<0.17.0.

The change updates formatter configuration and its changelog entry. It changes no package code.

Validation at 0bed4c6a2159b14c6d266e213277562c882b269d:

All eight reported GitHub checks/statuses passed, including lint, documentation, changelog, and the Python 3.12 and 3.14 test jobs.
A local ruff format --check . with Ruff 0.16.7 passed: 74 files already formatted.
git diff --check passed.

The Lint job installs ruff>=0.9.0 with no upper bound. ruff 0.16.7 formats Python inside markdown code blocks, which earlier versions left alone, so five documentation files under docs/ became unformatted without anyone changing them. make check-format fails on an untouched checkout of main, and therefore on every open pull request. Reformats the five files and gives the constraint an upper bound, so a future ruff release changes the lint result only when someone chooses to move the pin.

vercel · 2026-09-16T12:13:40Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
microimpute-dashboard	Ready	Preview	Sep 21, 2026 9:33am UTC

vahid-ahmadi · 2026-09-21T09:18:56Z

Reviewed with a fresh pass. The bug is real and current — merge this first. On today's origin/main with ruff 0.16.8: 5 files would be reformatted, 83 files already formatted. The branch is clean across the whole allowed range: ruff 0.9.0 gives 65 files already formatted, 0.16.0 and 0.16.8 both give 89 files already formatted. Lint is currently red on #200 and #201 for this reason alone, so landing this unblocks them.

Two things in the PR are wrong, though, and one of them defeats its own purpose.

To do

The version bound is a no-op against the failure it claims to prevent. "ruff>=0.9.0,<0.17.0" resolves to 0.16.8 — which formats markdown, as do 0.16.0 through 0.16.8, and all satisfy the bound. Only the reformatting fixes the job; the bound protects nothing, and it guarantees a repeat the next time a formatter detail changes inside 0.16.x. Either pin exactly (ruff==0.16.8) so CI is reproducible, or add a [tool.ruff] section — there is currently none in pyproject.toml — with exclude = ["docs/**/*.md"] so prose files stop being a lint surface. Pinning exactly is the smaller change; excluding markdown is the more durable one.
The stated cause is the wrong version. Bisected: 0.13.0, 0.14.0 and 0.15.0 all give "65 files already formatted" — markdown is not scanned at all — and 0.16.0 is where it becomes "5 files would be reformatted". Not 0.16.7. Correct the description and changelog.d/lint-on-main.fixed.md.
The dev extra is left unbounded, so local and CI ruff disagree. pyproject.toml:38 still has ruff>=0.9.0 while CI gets the constrained range, which means a contributor running make format on a newer ruff reformats files CI then rejects. Apply the same constraint or pin.

Merge order

#219 contains this PR byte-for-byte — same five docs files, same changelog fragment, same workflow hunk — and its body credits it. Merge this one separately and first regardless: it is ~20 lines and fixes a red check on main today, whereas #219 is a large draft. The rebase will be conflict-free since the content is identical.

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.

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.

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.

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.

@juaristi22

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.

juaristi22 · 2026-09-21T10:47:01Z

Review of 0bed4c6: ready to merge; no implementation defect found. All eight scheduled checks passed at this head.

The current implementation adds extend-exclude = ["docs/**/*.md"] to pyproject.toml and uses ruff>=0.16.0,<0.17.0 in both the development extra and Lint workflow. Markdown no longer enters the formatter surface; Python package files and Python sources under docs remain in scope. No runtime Python test rerun was needed for this configuration-only change; the actual Lint, documentation, unit/smoke, changelog, and deployment checks passed.

Update the PR description: it still says the PR reformats five Markdown files and sets >=0.9.0,<0.17.0. The final diff excludes Markdown and aligns both constraints at >=0.16.0,<0.17.0.

Merge this first to resolve the common Lint failure on #214–#217, then update those branches and rerun CI. Read-only merge checks found no textual conflicts among those five current heads; no combined-suite pass is claimed. #219 contains the earlier formatter implementation and now conflicts with this head, so preserve this final configuration when integrating #219.

juaristi22 · 2026-09-21T10:53:11Z

The review found no additional code fix necessary. The current implementation excludes docs/**/*.md and uses ruff>=0.16.0,<0.17.0 in both CI and the development extra. The earlier description of reformatting five Markdown files and using a >=0.9.0 lower bound no longer describes this implementation.

Verification

At 0bed4c6a2159b14c6d266e213277562c882b269d, all eight reported GitHub checks/statuses pass, including lint, documentation, changelog, and Python 3.12 and 3.14 tests. The local Ruff 0.16.7 format check also passes with 74 files already formatted; git diff --check passes.

The same lint configuration is being incorporated into #214–#217 so those branches can pass lint independently.

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.

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.

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.

@juaristi22

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.

…ta (#216) * Declare subpackages instead of shipping the source tree as package data 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 * Fix issues from review: format documentation examples * Add per-version classifiers, an author email, and a find exclude 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. --------- Co-authored-by: María Juaristi <127882282+juaristi22@users.noreply.github.com>

* Give each per-variable QRF model its own seed 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 * Fix issues from review: bound QRF seeds and preserve tuning streams * Fail loudly on an unknown variable and an invalid seed 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. --------- Co-authored-by: María Juaristi <127882282+juaristi22@users.noreply.github.com>

* Prune failed Matching trials instead of scoring the training mean 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 * Fix issues from review: track Matching prediction failures * Reset the failure counter, report the cause, expose it on the result 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. * Fix indentation that moved a raise out of its except block 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. * Fix issues from review: restore lint compatibility and preserve matching failure metadata --------- Co-authored-by: María Juaristi <127882282+juaristi22@users.noreply.github.com>

@juaristi22

…ter (#217) * Fix tests that pass for the wrong reason, and export ZeroInflatedImputer 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 * Allow Matching's own message in the missing-predictor test 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. * Fix issues from review: preserve public config compatibility * Act on review: loosen frozen tests, correct the weight docstring 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. --------- Co-authored-by: María Juaristi <127882282+juaristi22@users.noreply.github.com>

vercel Bot deployed to Preview September 16, 2026 12:14 View deployment

juaristi22 mentioned this pull request Sep 16, 2026

Fix pre-submission imputation and evaluation correctness #219

Draft

This was referenced Sep 21, 2026

Fix tests that pass for the wrong reason, and export ZeroInflatedImputer #217

Merged

Read the package version from installed metadata #200

Open

Add JOSS paper for submission #201

Draft

vercel Bot deployed to Preview September 21, 2026 09:33 View deployment

This was referenced Sep 21, 2026

Give each per-variable QRF model its own seed #214

Merged

Prune failed Matching trials instead of scoring the training mean #215

Merged

Declare subpackages instead of shipping the source tree as package data #216

Merged

juaristi22 merged commit 6d0eaa9 into main Sep 21, 2026
8 checks passed

Navigation Menu

Fix the Lint job, which is failing on main - #218

Fix the Lint job, which is failing on main#218
juaristi22 merged 2 commits into
mainfrom
fix/lint-on-main

vahid-ahmadi commented Sep 16, 2026 •

edited by juaristi22

Loading

Uh oh!

vercel Bot commented Sep 16, 2026 •

edited

Loading

Uh oh!

vahid-ahmadi commented Sep 21, 2026

Uh oh!

juaristi22 commented Sep 21, 2026

Uh oh!

juaristi22 commented Sep 21, 2026

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

vahid-ahmadi commented Sep 16, 2026 • edited by juaristi22 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel Bot commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vahid-ahmadi commented Sep 21, 2026

To do

Merge order

Uh oh!

juaristi22 commented Sep 21, 2026

Uh oh!

juaristi22 commented Sep 21, 2026

Verification

Uh oh!

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vahid-ahmadi commented Sep 16, 2026 •

edited by juaristi22

Loading

vercel Bot commented Sep 16, 2026 •

edited

Loading