Prune failed Matching trials instead of scoring the training mean

[vahid-ahmadi]

Fixes #210.

Matching tuning prunes trials when donor matching raises an error, including errors in later chunks. Failed trials cannot win model selection through fallback training means. If no trial completes, tuning raises a ValueError that includes the last matching error when available.

Chunked predictions retain successful rows and return missing values for unmatched rows. Each prediction resets n_failed_records and counts records with missing target values. Every returned prediction frame now includes attrs["n_failed_records"], for both default predictions and explicit quantiles; earlier frames retain their own counts after later calls.

Validation: the affected Matching manifest passes 12 tests, covering actual Optuna pruning, all-pruned studies, partial output, indexes, counter resets, and result metadata. The four metadata regressions first produced two failing default-prediction cases and two passing explicit-quantile controls. The optional R/rpy2 integration module skips locally; these local results do not validate the real R backend. Verification confirmed unchanged program and test contents after the base rebase. The branch includes #218's shared lint repair; Ruff 0.16.7 formatting and git diff --check pass.

Checks for current head 62d03ce.

[vercel]

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

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

[vahid-ahmadi]

Reviewed with a fresh pass. The change is correct and the new tests are the strongest part of it: tests/test_models/test_matching_failures.py sidesteps the missing rpy2 by reloading matching.py with a stubbed adapter and a deterministic Python backend, then asserts real Optuna state — trials[0].state == PRUNED, trials[1].state == COMPLETE — plus the all-pruned ValueError, index preservation and the counter reset. 8 passed. That is a genuine exercise of the changed lines rather than a smoke test.

Two claims in the description checked and confirmed: import optuna is function-local at matching.py:570 but the objective closure and both raises are nested inside _tune_hyperparameters, so it resolves; and an all-pruned study really does raise ValueError: No trials are completed yet. on best_value, which matching.py:735-738 correctly guards by checking for a COMPLETE trial before touching study.best_value.

To do

Before merging

• The counter goes stale when a prediction raises. matching.py:174-177 raises RuntimeError("Hot deck matching failed") before reaching _process_matching_results, so n_failed_records keeps the previous successful call's value. A caller doing try: predict() / except: read n_failed_records gets a stale number. Reset it at the top of _predict.
• The description's "raises an explicit Matching error" is not accurate. matching.py:547-549 catches everything and re-raises as ValueError(f"Failed to set up matching model: {e}"), so callers get a generic type indistinguishable from any other setup failure. Either raise a dedicated exception that fit re-raises unwrapped, or amend the description.

Worth doing

• Document and expose n_failed_records. It appears in zero docstrings and nothing under docs/, and is not in the return value. Fix pre-submission imputation and evaluation correctness #219 additionally sets output.attrs["n_failed_records"] — worth adopting that here rather than leaving it to the rebase.
• Include the underlying error in the final ValueError at matching.py:738. When matching fails structurally (bad dtype, missing R package) the user currently gets N warnings and a summary, with the root cause only reachable via __cause__.
• Split out the five docs/** formatter reflows — same hunks as Give each per-variable QRF model its own seed #214 and Declare subpackages instead of shipping the source tree as package data #216.

Optional

• Hoist import optuna to module scope, so a future use outside _tune_hyperparameters doesn't NameError.
• Note the single-threaded assumption in the docstring. autoimpute.py:226-230 forces n_jobs = 1 when a Matching model is present, so the counter is safe in practice, but concurrent predict calls on one fitted object would race.

On #219: it contains this fix and goes further — it also prunes on silently incomplete results (ValueError("Matching returned incomplete predictions")) and on nonfinite predictions, which this PR leaves open. But #219 is a draft and a large rewrite. Merging this first and rebasing keeps the reviewable unit; just make sure the rebase keeps #219's stricter incomplete/nonfinite checks rather than regressing to this narrower version.

Caveat worth recording: the changed lines have not been exercised against real StatMatch anywhere I could run. rpy2 is absent here and requireNamespace("StatMatch") is FALSE, so the pre-existing matching tests skip. Only the 3.14 CI job installs R.

[juaristi22]

Review of 626278c: The remaining merge blocker is the inherited Lint failure. One nonblocking result-metadata defect also needs correction.

Merge blocker — incorporate #218 and rerun CI. The Lint job fails because Ruff would reformat five unchanged Markdown files: docs/imputation-benchmarking/cross-validation.md, preprocessing.md, visualizations.md, docs/models/imputer/implement-new-model.md, and docs/use_cases/index.md. The workflow runs make check-format. #218 fixes the shared failure by excluding Markdown and aligning the development and CI Ruff ranges at >=0.16.0,<0.17.0. Merge #218 first, update this branch, and require green checks on the resulting head.

Nonblocking — attach n_failed_records to default prediction results. In matching.py:392–396, metadata assignment only runs when explicit quantiles are supplied. With three receiver rows and a backend returning one missing target, fitted.predict(X) returns one NaN and fitted.n_failed_records == 1, but result.attrs == {}. predict(X, quantiles=[0.5]) returns attrs["n_failed_records"] == 1. Set the attribute on every returned frame and cover both public return paths. #219 already constructs this metadata correctly; preserve that behavior during integration.

Validation at the reviewed head: 71 affected tests passed; the optional real-R Matching module skipped locally. Deterministic backends exercised actual Optuna pruning, all-pruned studies, partial output, index preservation, counter resets, and missing-target-only counting. A separate public-API reproduction confirmed the metadata discrepancy above. Unit/smoke and changelog CI passed; Lint failed.

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.

[juaristi22]

Critical fixed

Updated the branch onto current main, incorporating #218's shared lint repair and the 3.1.2 release.

Should-address fixed

Default predict(X) now attaches attrs["n_failed_records"] to its returned frame, matching explicit-quantile predictions. Four behavioral regressions cover zero/one unmatched row, default/multiple quantiles, index and value preservation, counter resets, and metadata retained on earlier frames. The description now specifies the all-pruned ValueError and current validation evidence.

Verification

• Before the metadata fix, two default-prediction cases failed and two explicit-quantile controls passed. The affected Matching manifest now passes 12 tests, with one optional R/rpy2 integration module skipped locally.
• Those local tests exercise deterministic matching backends and real Optuna pruning; they do not establish real-R integration coverage.
• Source and base-integration verification passed. The rebase preserved program and test contents.
• Ruff 0.16.7 formatting and git diff --check passed; make format changed no files.

Pushed head: 62d03ce; current-head checks.

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).