You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Small items found during the pre-JOSS-submission audit (#201), each verified by a second reviewer. Grouped because none warrants its own issue; several are one-line fixes.
Public API and defaults
ZeroInflatedImputer is not exported. 717 lines, two dedicated test files, 79% coverage, its own __all__ — but hasattr(microimpute, "ZeroInflatedImputer") and hasattr(microimpute.models, "ZeroInflatedImputer") are both False from an installed wheel, and it appears nowhere in docs/, README.md or examples/. Only from microimpute.models.zero_inflated import ZeroInflatedImputer works. paper.md describes it as a feature. Fix: export it and add it to the "Available models" list in models/__init__.py:6-13.
autoimpute() cannot reach four Imputer.fit parameters.not_numeric_categorical, skip_missing, row_filter and target_filters exist at imputer.py:241-256 but not in the autoimpute() signature (autoimpute.py:359-377). not_numeric_categorical is the escape hatch from the < 10 distinct values → categorical reclassification, which is announced only at logger.info (type_handling.py:101) while autoimpute defaults to log_level="WARNING", so it is invisible. Related to Log loss is computed from class labels, and integer counts are silently switched to classification #209.
Silent wrong answer on a dtype mismatch. If a receiver predictor column is int where training was float, every row silently falls back to the reference category with a UserWarning (imputer.py:574) rather than erroring.
Validation
validate_imputation_inputs (validation.py:85-113) only checks column existence. Two cases are genuinely unhelpful: duplicate predictor names give RuntimeError: Failed to preprocess data types with no detail, and a receiver predictor typed str against a donor floatcompletes silently with no error at all. A predictors/imputed_variables overlap reports Missing columns in receiver data, which points at the wrong cause.
validation.py also has no test file — grep -rn validate_imputation_inputs tests/ returns nothing.
CI
MDN tests are gated out..github/workflows/pr_code_changes.yaml:23-46 greps the changed-file list for (mdn|MDN) and installs .[dev,matching,mdn] only on a match (line 88) versus .[dev,matching] otherwise (line 83). With pytest.importorskip("pytorch_tabular") at test_mdn.py:15, a breaking change to imputer.py, type_handling.py or autoimpute.py merges green against 456 statements of MDN. Fix: always run a small MDN smoke test, and gate only the full suite.
ruff check never runs.make check-format is ruff format --check . only; main.yml has no lint job at all. There is no [tool.ruff] section anywhere, so nothing is configured. Formatting is clean, but ruff check --select E4,E7,E9,F reports 160 errors on the core rule set, including 96 unused imports and 6 unused variables. (The full default rule set reports 1148, mostly style modernisation — the core number is the meaningful one.)
Tests that cannot fail
tests/test_models/test_qrf.py:763-766 wraps the call in try/except and asserts inside the except branch, so if predict ever stops raising the test passes having asserted nothing. Use pytest.raises(RuntimeError, match=...).
Four pytest.raises(Exception) with no match= (test_autoimpute.py:375, :395, test_models/test_imputers.py:723, test_models/test_qrf.py:750) pass on any exception, including a NameError from a typo — which is exactly how the Test suite fails with 8 errors on a clean checkout without rpy2 #204 failures could have been masked.
tests/test_models/test_imputers.py:684-694 calls a private method and asserts nothing.
Dead and misplaced code
config.py:43-77 — DEFAULT_MODEL_PARAMS, 35 lines, zero references in the repo. Free to drift out of sync with the real defaults while reading as authoritative.
config.py:18-31 — VALID_YEARS, a hardcoded list of SCF survey years, used only by notebooks. A model-agnostic package should not carry it in public config.
Docs and examples
examples/pipeline.py:276 hardcodes output_path="microimputation-dashboard/public/microimputation_results.csv", a tracked file. Run from the repo root the documented example dirties the working tree; run from anywhere else it raises OSError: Cannot save file into a non-existent directory. main.yml:53-55 runs it on push to main.
models/imputer.py:264 — the fit docstring still describes the removed bootstrap-resampling weight scheme ("sampled with replacement using this column as selection probabilities"). The code at :397-410 passes sample_weight to each learner's native weighted fit instead. Users will expect resampling variance that no longer exists.
comparisons/metrics.py:634-637 — the compare_distributions docstring example is wrong on both printed values and takes a different code path than documented. Running it verbatim gives income kl_divergence 21.93 and region 0.0, against the documented income wasserstein 66.67 and region kl 0.167.
mdn.py:850 — model_dir: str = "./microimpute_models" writes trained-model caches into the caller's working directory. .gitignore already carries a microimpute_models/ entry, which suggests this has leaked into the repo before. Use platformdirs.user_cache_dir.
Dependencies
psutil (pyproject.toml:27) is the only unbounded dependency, and its single use (qrf.py:17-21, :835-836) is already guarded by try/except ImportError. Either move it to an extra or pin it.
kaleido>=0.2.1,<0.3.0 in the images extra resolves to 0.2.1, which warns that support for Kaleido below 1.0.0 "will be removed after September 2025". Bump to >=1.0.0 or use plotly[kaleido].
Small items found during the pre-JOSS-submission audit (#201), each verified by a second reviewer. Grouped because none warrants its own issue; several are one-line fixes.
Public API and defaults
ZeroInflatedImputeris not exported. 717 lines, two dedicated test files, 79% coverage, its own__all__— buthasattr(microimpute, "ZeroInflatedImputer")andhasattr(microimpute.models, "ZeroInflatedImputer")are bothFalsefrom an installed wheel, and it appears nowhere indocs/,README.mdorexamples/. Onlyfrom microimpute.models.zero_inflated import ZeroInflatedImputerworks.paper.mddescribes it as a feature. Fix: export it and add it to the "Available models" list inmodels/__init__.py:6-13.autoimpute()cannot reach fourImputer.fitparameters.not_numeric_categorical,skip_missing,row_filterandtarget_filtersexist atimputer.py:241-256but not in theautoimpute()signature (autoimpute.py:359-377).not_numeric_categoricalis the escape hatch from the< 10 distinct values → categoricalreclassification, which is announced only atlogger.info(type_handling.py:101) whileautoimputedefaults tolog_level="WARNING", so it is invisible. Related to Log loss is computed from class labels, and integer counts are silently switched to classification #209.intwhere training wasfloat, every row silently falls back to the reference category with aUserWarning(imputer.py:574) rather than erroring.Validation
validate_imputation_inputs(validation.py:85-113) only checks column existence. Two cases are genuinely unhelpful: duplicate predictor names giveRuntimeError: Failed to preprocess data typeswith no detail, and a receiver predictor typedstragainst a donorfloatcompletes silently with no error at all. A predictors/imputed_variables overlap reportsMissing columns in receiver data, which points at the wrong cause.validation.pyalso has no test file —grep -rn validate_imputation_inputs tests/returns nothing.CI
.github/workflows/pr_code_changes.yaml:23-46greps the changed-file list for(mdn|MDN)and installs.[dev,matching,mdn]only on a match (line 88) versus.[dev,matching]otherwise (line 83). Withpytest.importorskip("pytorch_tabular")attest_mdn.py:15, a breaking change toimputer.py,type_handling.pyorautoimpute.pymerges green against 456 statements of MDN. Fix: always run a small MDN smoke test, and gate only the full suite.ruff checknever runs.make check-formatisruff format --check .only;main.ymlhas no lint job at all. There is no[tool.ruff]section anywhere, so nothing is configured. Formatting is clean, butruff check --select E4,E7,E9,Freports 160 errors on the core rule set, including 96 unused imports and 6 unused variables. (The full default rule set reports 1148, mostly style modernisation — the core number is the meaningful one.)Tests that cannot fail
tests/test_models/test_qrf.py:763-766wraps the call intry/exceptand asserts inside theexceptbranch, so ifpredictever stops raising the test passes having asserted nothing. Usepytest.raises(RuntimeError, match=...).pytest.raises(Exception)with nomatch=(test_autoimpute.py:375,:395,test_models/test_imputers.py:723,test_models/test_qrf.py:750) pass on any exception, including aNameErrorfrom a typo — which is exactly how the Test suite fails with 8 errors on a clean checkout without rpy2 #204 failures could have been masked.tests/test_autoimpute.py:479-484skips its assertion when both losses are NaN, so the determinism test passes vacuously in precisely the case Matching silently substitutes the training mean on failure, and that score is fed to Optuna #210 produces.tests/test_models/test_imputers.py:684-694calls a private method and asserts nothing.Dead and misplaced code
config.py:43-77—DEFAULT_MODEL_PARAMS, 35 lines, zero references in the repo. Free to drift out of sync with the real defaults while reading as authoritative.config.py:18-31—VALID_YEARS, a hardcoded list of SCF survey years, used only by notebooks. A model-agnostic package should not carry it in public config.Docs and examples
examples/pipeline.py:276hardcodesoutput_path="microimputation-dashboard/public/microimputation_results.csv", a tracked file. Run from the repo root the documented example dirties the working tree; run from anywhere else it raisesOSError: Cannot save file into a non-existent directory.main.yml:53-55runs it on push to main.models/imputer.py:264— thefitdocstring still describes the removed bootstrap-resampling weight scheme ("sampled with replacement using this column as selection probabilities"). The code at:397-410passessample_weightto each learner's native weighted fit instead. Users will expect resampling variance that no longer exists.comparisons/metrics.py:634-637— thecompare_distributionsdocstring example is wrong on both printed values and takes a different code path than documented. Running it verbatim givesincome kl_divergence 21.93andregion 0.0, against the documentedincome wasserstein 66.67andregion kl 0.167.mdn.py:850—model_dir: str = "./microimpute_models"writes trained-model caches into the caller's working directory..gitignorealready carries amicroimpute_models/entry, which suggests this has leaked into the repo before. Useplatformdirs.user_cache_dir.Dependencies
psutil(pyproject.toml:27) is the only unbounded dependency, and its single use (qrf.py:17-21,:835-836) is already guarded bytry/except ImportError. Either move it to an extra or pin it.kaleido>=0.2.1,<0.3.0in theimagesextra resolves to 0.2.1, which warns that support for Kaleido below 1.0.0 "will be removed after September 2025". Bump to>=1.0.0or useplotly[kaleido].