Skip to content

Pre-submission cleanup: API exports, validation, CI gaps, vacuous tests, dead code, stale docs #213

Description

@vahid-ahmadi

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 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-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_autoimpute.py:479-484 skips 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-694 calls a private method and asserts nothing.

Dead and misplaced code

  • config.py:43-77DEFAULT_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-31VALID_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:850model_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].

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions