Add JOSS paper for submission - #201
vahid-ahmadi wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Reviewed as a JOSS reviewer would, checking the paper's assertions against the code rather than reading them. The mechanics are in good shape — 1,324 words, all required sections present, all 14 bib keys cited with no orphans, every DOI present resolving 200, But there is a hard blocker and several claims the current code contradicts. Hold this until they are resolved. Blocker
Claims the code contradicts
Repository requirements a reviewer checks
Smaller
Suggested order#218 → #200 → #217 → rebase and land #219 → regenerate benchmarks → revise and merge this. The Lint failure here is inherited from main, not this PR. |
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.
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.
…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>
The paper is a short software paper, distinct from the working paper in paper/, which it cites as the validation study. It is framed around the finding that motivates the package: benchmarking across seven domains shows no imputation method dominates, so the useful tool is one that measures which performs best on a user's own data. Also adds a citation file, a code of conduct, and the workflow that builds a draft PDF, none of which the repository had. The author list and ORCIDs still need confirming, and the missing licence in issue #197 blocks submission. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- fit/predict signatures matched to the actual API - weights claim narrowed to OLS and matching - six benchmark datasets, not seven domains, with the source's hedge - optional extras (rpy2/StatMatch, PyTorch) disclosed - statsmodels and R mice weight columns corrected to No - dropped the uk-public-services-imputation claim (declares microimpute but never imports it)
Adds the donor-receiver framing, the autoimpute workflow end to end, and the predictor-analysis utilities. Cites the policyengine JOSS paper in the research impact statement and links the PolicyEngine website.
compute_predictor_correlations reports association, not contribution; only leave_one_out_analysis and progressive_predictor_inclusion measure contribution by loss.
…claims A line-by-line check against the code found three statements the package does not support. The standardisation claim was false. autoimpute_helpers.py:147 captures the donor's transform parameters and :163 discards them with ', _', re-fitting on the receiver - issue #202. A reproduction with a donor mean of 100 and a receiver shifted to a true mean of 149 returned 99.73, the donor mean exactly, so the level difference imputation exists to carry was annihilated. The sentence now claims only consistent categorical encoding and describes the numeric transformations as optional, which is what the code does. The survey-weights roster was wrong in both directions: it omitted QRF, which does accept a weight column, and credited matching with weighted donor selection, which is unverified - statmatch_hotdeck.py passes weight_don to R's NND.hotdeck, which has no such parameter. Matching is dropped from the list until that is settled with rpy2 installed. 'Every method returns quantiles of the conditional distribution' is the paper's central premise and is false as shipped: QuantReg.predict with an explicit grid raises RuntimeError (#206). Reworded to the weaker claim the code supports. Also: StatMatch's survey-weights cell goes Yes to Partly for the same NND.hotdeck reason, with the distinction explained in the prose; the log-loss claim is softened because #209 means it is not a proper scoring rule; 'distributed separately' dropped, since the dashboard lives in this repository; and the benchmarking sentence now records that matching achieves the lowest mean rank overall, which the working paper reports and the previous wording omitted. The comparison table is transposed so criteria are rows and tools are columns, with citations moved into the paragraph below - the tool names and their citations were forcing five-line-tall rows. The figure follows the policyengine.py JOSS paper's house style: a hand-written SVG in the same palette, committed alongside its PNG, in the Software Design section.
e8efaf3 to
556c117
Compare
At full width the portrait diagram dominated the Software Design page.
Summary
paper.mdandpaper.bibfor submission to the Journal of Open Source SoftwareCITATION.cff,CODE_OF_CONDUCT.mdand.github/workflows/draft-pdf.yml, none of which the repository hadFollows PolicyEngine/policyengine.py#264, which was accepted and published.
This is a different paper from
paper/paper/main.texis a 39-page empirical study — SCF-to-CPS wealth imputation, an SSI policy simulation, cross-dataset benchmarking. JOSS wants a short software paper and will desk-reject a research article, so this is a new, software-focusedpaper.mdthat cites the working paper as the validation study rather than replacing it.The framing leads on the finding that justifies the package existing: benchmarking across six datasets alongside the wealth application shows no method dominating — QRF wins on nonlinear relationships, while matching achieves the lowest mean rank overall because it better preserves marginals. If performance is dataset-specific, the useful contribution is a harness that measures it, which is a stronger claim than "five imputation methods".
JOSS requirements
paper.mdwith Summary, Statement of Need, State of the Field, Software Design, Research Impact Statement, Acknowledgements, AI Usage Disclosure, Referencespaper.bib— 14 entries, all cited, no orphans, all DOIs resolveCITATION.cff, validated against schema 1.2.0CODE_OF_CONDUCT.md0009-0007-4946-2248, verified against the ORCID registryBlockers before submitting
Must fix — correctness
A pre-submission audit of the models turned up a set of silent correctness bugs. A reviewer will run these methods, and they produce plausible wrong numbers rather than errors. A second round of review, with every finding independently reproduced by an adversarial verifier before filing, added #207-#213.
QuantReg.fitonly fits the median,Matchingreturns identical values for every quantile, intercepts dropped on constant predictors, weights dropped in two places,train_sizeandrandom_stateinert8 failed, 4 passedwithout rpy2 becauseMatchingwas named unconditionally; it now builds the model list from what importedQRF(seed=)validates at constructioncross_validate_modelon the same model and data (2.14 against 0.79, truth 0.69). Integer targets with fewer than ten levels are also silently switched to classificationn_failed_recordsis reset per call and mirrored ontoresult.attrsautoimputenever reaches itMust fix — compliance
LICENSEfile, no licence in packaging metadata. This is the first item on the JOSS checklist, and it also means the five downstream repositories currently depend on all-rights-reserved code__version__reported 1.1.2 while the package was 3.1.1 (PR open, rebased and green; fallback is now PEP 440 parseable and tested)py.typed, the full author list, per-version classifiers and project URLs. The licence metadata half stays with microimpute has no licence, so all rights are reserved #197.github/CONTRIBUTING.mdbeyond its current 12 lines to cover dev setup, running tests, reporting issues, and where to seek supportShould fix — reviewers will raise these
>=3.12,<3.15range; 3.13 never runs on PRs, 3.14 never on main, and the full suite effectively runs on one versionpolicyengine-us-data, open since Octobercompute_predictor_correlationsdocuments a normalized mutual information matrix, but divides MI in nats by an entropy in bits whose continuous-column value islog2(n_distinct). Two identical variables score 0.55 rather than 1.0, and the value drifts with sample sizeZeroInflatedImputeris undocumented and unexported although the paper describes it;autoimpute()cannot reach fourImputer.fitparameters;validation.pyhas no test file; MDN tests are gated out of CI by a changed-filename grep; five tests cannot fail or assert nothing; dead config; and four stale docstrings, including acompare_distributionsexample that is wrong on both printed valuesWorth considering
microimputation-dashboard/to its own repository — a Next.js app roughly doubles the review surface for something that is not the Python packagepaper/clarifying that the manuscript is an unpublished working paper distinct from this submissionAn honest note on impact
Every dependent is inside the PolicyEngine organisation:
policyengine-uk-data,uk-trade-shock-study,uk-public-services-imputation,nics-exemption-inactive-employees,imputation-paper. There is no identifiable external user, and the repository has 0 stars. JOSS does not require external adoption but reviewers weigh it. The strongest honest framing is production dependency — a microsimulation data pipeline depends on this — rather than community uptake, and that is how the paper puts it.Authorship to confirm
Currently María (corresponding), Max, Nikhil, Vahid — by commit share that is 354 / 101 / 45 / small. @baogorek contributed 15 commits and is acknowledged rather than listed; worth confirming that is right with him.