Repository navigation
Give each engine's element set one source - #167
Merged
Merged
Conversation
The ANI element set was written five times across three layers, all hand-maintained, with nothing connecting them: the numeric frozenset in `models/policy.py` (the gate that raises ConfigurationError), the keys of `ANI2XT_INDEX` in `models/species.py` (the remap that raises ValueError), the symbol string in that remap's error message, and two more copies of that string in the CLI's `ENGINE_INFO`. All five agreed. They agreed by hand, which is the same arrangement the registry replaced for engine *names* -- correct until someone edits one of them, and `auto3d models info` is exactly where a user checks which elements an engine accepts before choosing it. `format_elements` renders an element set as symbols ordered by atomic number. That order is not a new convention: it is the order all six existing strings already used, so no user-visible output changes -- which is what makes the string tests behavior locks rather than churn. It lives in `species.py` with rdkit imported inside the function body, preserving that module's documented lazy-rdkit property, and so that the dependency runs policy -> species with no sibling cycle. The gate and the remap are now connected by an import-time assert rather than by derivation. They are the same seven numbers but not the same fact: `ANI_ELEMENTS` is what ANI2x AND ANI2xt were trained on, `ANI2XT_INDEX` is one engine's 0-based network index order. Defining either in terms of the other would record a provenance that is not true and would stop being a check. Same construction as model_factory's BUILTIN_ANI_MODELS assert. AIMNet2 is handled differently because the knowledge is not Auto3D's. Each model file declares `implemented_species`, and `AIMNet2Calculator` enforces it at call time -- so Auto3D does not define these sets, it only quotes them. The four literals therefore stay literals (deriving them would mean loading four NNPs to print a help table) and a new slow-tier test pins them to the metadata instead. They are correct today: all four match, including aimnet2-pd's As -> Pd substitution. Wave 6's third bullet, `energy_unit`, is disposed of rather than dropped: `utils/energy.py` already owns it completely -- E_tot is Hartree on disk, models produce eV, one module converts, and the adapter contract's docstrings state eV at every boundary. No adapter member is needed. Also recorded, not acted on: all four aimnet registry models report `supports_charged_systems = None`, so aimnet does not enforce a charge restriction for them and Auto3D's own `_requires_aimnet` charge test is the only charge guard on that path. That is existing, correct behavior. Nothing here touches `ModelAdapter`. `check_engine_supports_molecules` runs before any model is constructed, from callers holding only an engine name, so "ask the adapter" is the same chicken-and-egg left behind in D4 -- and for AIMNet2 there is nothing to ask, since the calculator validates itself. Verification: - 1766 passed, 1 skipped, 74 deselected (+5 fast, +4 slow), randomized order. - The 4 slow tests pass locally on CPU in 33s, so CI's slow tier pays seconds, not a download. - mypy unchanged: 68 errors in 21 files, "checked 72 source files". No new error in any touched file. - Mutation-tested both guards. Widening `ANI_ELEMENTS` to include bromine without touching the remap fails the import-time assert, naming [35]. Sorting the renderer by symbol instead of by atomic number fails three string tests, including the two that pin `ENGINE_INFO`.
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
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.
The problem
The ANI element set was written five times across three layers, all hand-maintained, with nothing connecting them:
models/policy.pyfrozenset({1, 6, 7, 8, 9, 16, 17})ConfigurationErrormodels/species.pyANI2XT_INDEXValueErrormodels/species.py"(supported: H, C, N, O, F, S, Cl)"cli/commands/models.py"elements": "H, C, N, O, F, S, Cl"×2auto3d models infoprintsAll five agreed — by hand. That is the same arrangement the registry replaced for engine names: correct until someone edits one of them. And
auto3d models infois precisely where a user checks which elements an engine accepts before choosing it, so a stale copy there misinforms the decision it exists to support.The change
format_elementsrenders an element set as symbols ordered by atomic number. That order is not a new convention — it is the order all six existing strings already used, so no user-visible output changes. That is what makes the string tests behavior locks rather than churn.It lives in
species.pywith rdkit imported inside the function body, preserving that module's documented lazy-rdkit property and keeping the dependencypolicy → specieswith no sibling cycle.The gate and the remap are connected by an import-time assert, not by derivation. They are the same seven numbers but not the same fact:
ANI_ELEMENTSis what ANI2x and ANI2xt were trained on;ANI2XT_INDEXis one engine's 0-based network index order. Defining either in terms of the other would record a provenance that is not true, and would quietly stop being a check. Same construction asmodel_factory'sBUILTIN_ANI_MODELSassert.AIMNet2 is handled differently, because the knowledge isn't Auto3D's
Each model file declares
implemented_species, andAIMNet2Calculatorenforces it at call time. Auto3D doesn't define these sets — it only quotes them. So the four literals stay literals (deriving them would mean loading four NNPs to print a help table), and a new slow-tier test pins them to the metadata instead.They are correct today: all four match, including
aimnet2-pd's As → Pd substitution.Wave 6's third bullet, disposed of rather than dropped
energy_unitneeds no adapter member.utils/energy.pyalready owns it completely —E_totis Hartree on disk, models produce eV, one module converts, and the adapter contract's docstrings state eV at every boundary.Recorded, not acted on
All four aimnet registry models report
supports_charged_systems = None, so aimnet does not enforce a charge restriction for them and Auto3D's own_requires_aimnetcharge test is the only charge guard on that path. Existing, correct behavior — noted so it isn't rediscovered as a surprise.Not touched:
ModelAdaptercheck_engine_supports_moleculesruns before any model is constructed, from callers holding only an engine name — so "ask the adapter" is the same chicken-and-egg deliberately left behind in D4. And for AIMNet2 there is nothing to ask, since the calculator validates itself.Verification
ANI_ELEMENTSto include bromine without touching the remap fails the import-time assert, naming[35]. Sorting the renderer by symbol instead of by atomic number fails three string tests, including the two that pinENGINE_INFO.