refactor!: retire EnForce_ANI's type-switched second parameter - #136
Merged
Conversation
BREAKING CHANGE: EnForce_ANI's signature is now (model_adapter, batchsize_atoms=16384). The second parameter used to be `name_or_batchsize: str | int | None`, switching between a model name and a batch size; passing a string warned it would be "removed in Auto3D v2.0". The package reached 3.0.0 with it still there -- two majors past its own removal notice -- and no caller in src/ ever passed one. `_legacy_forward`, 55 lines dispatching on `self.name`, is deleted with it. Verified before acting, per the practice this backlog now requires: the three production callers are batchopt.py:319 (positional int), ASE/thermo.py:590 and SPE.py:148 (adapter only). None uses the string form, and the positional int call keeps working unchanged against the two-parameter signature. Removing the union on its own would have left a trap for exactly the callers the removal affects: `EnForce_ANI(adapter, "AIMNET")` would assign a string to batchsize_atoms and surface much later inside batching as a comparison error naming neither the parameter nor the removal. It now raises TypeError on the spot, saying what the parameter is for and where to get an adapter. Two tests asserted `_use_legacy_forward is False` while actually testing batch-size handling; they keep their real assertions and lose the one about a deleted attribute. The test of the removed feature is replaced by one asserting the old call shape is now rejected. Mutation-verified: dropping the guard fails it. Verified: 1273 passed, 9 skipped; ruff clean.
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.
One of the three M53 entries left unresolved by #135 — and the clearest of them, since the deprecation window had demonstrably expired.
What was there
One parameter, two meanings, switched on type: a model name (the pre-adapter API) or a batch size. Passing a string warned it would be "removed in Auto3D v2.0". The package reached 3.0.0 with it still in place — two majors past its own removal notice — along with
_legacy_forward, 55 lines dispatching onself.name.Verified before acting
Per the practice this backlog now requires, the three production callers were checked rather than assumed:
batch_opt/batchopt.py:319ASE/thermo.py:590SPE.py:148None uses the string form, and the positional-int call keeps working unchanged against the new two-parameter signature — so no call site needed editing.
The trap removing it would have left
EnForce_ANI(adapter, "AIMNET")— the exact shape a migrating caller would write — would have silently assigned a string tobatchsize_atomsand failed much later inside batching, as a comparison error naming neither the parameter nor the removal.It now raises
TypeErroron the spot, saying what the parameter is for and where to get an adapter. That guard is the difference between a breaking change and a confusing one.Signature
Migration: build an adapter with
Auto3D.model_factory.create_modeland pass it first.Tests
Two tests asserted
_use_legacy_forward is Falsewhile actually testing batch-size handling — they keep their real assertions and lose the one about a deleted attribute. The test of the removed feature is replaced by one asserting the old call shape is rejected; mutation-verified, dropping the guard fails it.I also caught myself here: my first edit blind-replaced the deleted assertion, appending
== 1024to tests that already asserted== 512and== 256. The suite failed immediately and it was fixed before commit, but a blind string replace across tests was the wrong tool.1273 passed, 9 skipped; ruff clean.
Still open from M53
ASE/thermo's unreadmodel_nameparameter, andexceptions.py's "4 classes never raised" — the latter needs finding by content, since line 41 isOptimizationError, raised three times.