Repository navigation
Pre-submission cleanup: API exports, validation, CI gaps, vacuous tests, dead code, stale docs #213
Copy link
Copy link
Open
Description
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.- Silent wrong answer on a dtype mismatch. If a receiver predictor column is
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 give RuntimeError: Failed to preprocess data types with no detail, and a receiver predictor typed str against a donor float completes 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-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=...).- Four
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].
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels
Activity