refactor!: delete the dead code M53 was right about, after checking all of it - #135
Merged
Conversation
…ll of it BREAKING CHANGE: `Auto3D.utils_file` is removed, along with `Auto3D.utils.encode_smiles`, `decode_smiles` and `housekeeping_helper` (which were in `utils/__init__.__all__`), `cli.results.count_from_output`, and the `BOND_STRETCH_TOLERANCE`, `COLLISION_THRESHOLD` and `SUPPORTED_MODELS` constants. None had a production caller. Per the release decision, deleted outright rather than shimmed. Re-verifying M53 before acting on it was the whole point, and it was justified: only 4 of its 13 entries described code that was both present and dead. - Three were already deleted by earlier phases: pad_molecular_batch, create_progress, IsomerProgressCallback -- zero hits in src/. - Six are not dead. utils/stereo_check has six live uses; ASE/thermo's mol2atoms and STANDARD_PRESSURE were revived by this session's own thermo work; isomers/parallel_embed was wired to Auto3DOptions in the previous change; and cli/results' FailedMolecule and print_failures are called from run.py, so the finding's supporting claim -- "run.py:149 admits failures is always []" -- stopped being true when the C6/C7 reconciliation landed. - Three cannot be resolved from the finding at all: its line numbers are stale. exceptions.py:41 is OptimizationError, which is raised three times, so the cited lines no longer point at the four never-raised classes described. Those, the model_wrapper legacy `name` API (still warning "removed in Auto3D v2.0" at version 3.0.0) and ASE/thermo's unread model_name param need their own pass. Net 256 lines out of src/ and 167 out of tests/, against a claimed ~450 in src/ -- the gap is a third already gone and half of the remainder alive. Every deleted symbol's tests went with it, which is why this ran before the test-hardening step: four test classes and two test functions are gone rather than being improved. Verified: 1273 passed, 9 skipped; ruff clean.
This was referenced Aug 3, 2026
isayev
added a commit
that referenced
this pull request
Aug 3, 2026
No code change. Both remaining dead-code claims were checked against source and neither holds. ASE/thermo's `model_name` parameter is read: Calculator stores it and passes it to to_model_species at thermo.py:536. It was genuinely unused when the audit was written; the C3/C4 species-conversion work made it live. exceptions.py's "4 classes never raised" is wrong, and the shape of the claim is why. Only ModelError is never raised directly, and that is deliberate: ModelLoadError and NumericalError subclass it, and cli/errors.py maps ModelError to exit code 5 precisely so both subclasses inherit that code. Deleting it would break the exit-code scheme this release documents. "Never raised" is not "unused" for a base class, and an audit that greps for `raise X` cannot tell the two apart. Final tally for M53: of 13 entries, 4 were real. Three had already been done, six were wrong on re-check, and the four real ones were removed in #135 and #136. Nothing further to delete. The lesson worth keeping is that a dead-code finding decays faster than any other kind. The same work that fixes defects revives symbols the audit recorded as dead -- mol2atoms, STANDARD_PRESSURE, FailedMolecule, print_failures and now model_name were all dead when written up and are all live now. A mechanical sweep of this list would have deleted five live symbols, two of which vib_hessian depends on. Both corrections are also written inline in the audit manifest, so neither can be re-derived from its finding ID.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
B2 of the debt plan (#130). Re-verifying M53 before acting on it was the point, and it was justified: only 4 of its 13 entries described code that was both present and dead.
pad_molecular_batch,create_progress,IsomerProgressCallback— zero hits insrc/utils/stereo_check(6 live uses),cli/resultsFailedMolecule+print_failures,ASE/thermomol2atoms,STANDARD_PRESSURE,isomers/parallel_embedutils_file.py(whole module),count_from_output,encode_smiles,decode_smiles,housekeeping_helper, and 3 constantsexceptions.py"4 classes never raised",model_wrapperlegacynameAPI,ASE/thermounreadmodel_nameparamTwo of the "not dead" entries are worth naming, because they show why a stale audit is dangerous to sweep from:
mol2atomsandSTANDARD_PRESSUREwere revived by this session's own thermo work — both were genuinely dead when the audit was written, and a mechanical sweep would have deleted code thatvib_hessiannow depends on.FailedMoleculeandprint_failuresrested on the finding's supporting claim that "run.py:149admitsfailuresis always[]". That stopped being true when the C6/C7 reconciliation landed.run.py:281constructsFailedMoleculeand:301callsprint_failures.What was actually deleted
BREAKING:
Auto3D.utils_file(whole module, zero importers anywhere),Auto3D.utils.encode_smiles,decode_smiles,housekeeping_helper(all three were inutils/__init__.__all__),cli.results.count_from_output, and theBOND_STRETCH_TOLERANCE,COLLISION_THRESHOLD,SUPPORTED_MODELSconstants. None had a production caller. Deleted outright rather than shimmed, per the release decision.Net: 256 lines out of
src/, 167 out oftests/— against the finding's claimed ~450 insrc/. The gap is a third already gone and half the remainder alive.Why this ran before the test hardening
Four test classes and two test functions went with the code rather than being improved. That is the ordering rationale from the plan, and the reason the plan's own sequence was corrected: hardening a test whose subject is about to be deleted is wasted work.
Verification
src/for real callers, with self-references and__all__barrel entries excluded — a raw count would have calledencode_smileslive on the strength of its own docstring examples.Left for a later pass
The three unresolved entries need finding by content, not by the audit's line numbers. Notably
model_wrapper's legacynameAPI still warns "deprecated and will be removed in Auto3D v2.0" — at version 3.0.0, two majors past its own removal notice.