diff --git a/CHANGELOG.md b/CHANGELOG.md index d56942bb..c0af4c72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to Auto3D will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [4.0.0] - unreleased +## [3.0.0] - unreleased ### Breaking Changes @@ -1427,7 +1427,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 surface as unrelated failures later. Twelve seeds now give identical pass and skip counts. -## [3.5.0] - 2026-06-13 +## [3.5.0-dev] - 2026-06-13 — never published + +> Tagged `v3.5.0` in git and never released to any package channel. Retained as +> a development record; nothing below reached a user through pip or conda. ### Breaking Changes @@ -1530,7 +1533,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Dead `torch.jit.optimized_execution` guard in the batch optimizer (a no-op for the eager-mode model wrapper). -## [3.0.0] - 2026-01-02 +## [3.0.0-dev] - 2026-01-02 — never published + +> Tagged `v3.0.0` in git and never released to any package channel. The version +> number is reused by the release above, which is the 3.0.0 users actually get: +> PyPI went 2.3.1 -> 3.0.0 and conda-forge 2.3.0 -> 3.0.0, with no 3.x in +> between. Retained as a development record. ### Breaking Changes @@ -1626,13 +1634,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Various bug fixes and stability improvements - Better handling of edge cases in stereoisomer enumeration -## [2.2.10] - 2024-03-29 +## [2.3.1] - 2024-08-13 + +- Published to PyPI. No CHANGELOG entry was written at the time; recorded here + so the file and the package index agree. + +## [2.3.0] - 2024-08-02 + +- Published to PyPI and conda-forge. No CHANGELOG entry was written at the time; + recorded here for the same reason. + +## [2.2.10] - 2024-03-29 — never published + +> Present in this file but absent from PyPI, which went 2.2.9 -> 2.3.0. ### Fixed - Minor bug fixes ## [2.2.9] - 2024-03-15 +> PyPI records this upload as 2024-02-13. The date above is as originally +> written; the discrepancy is noted rather than overwritten. + ### Changed - Performance improvements diff --git a/docs/superpowers/follow-ups-after-4.0.0-remediation.md b/docs/superpowers/follow-ups-after-4.0.0-remediation.md index 50f9235f..a6d5e99a 100644 --- a/docs/superpowers/follow-ups-after-4.0.0-remediation.md +++ b/docs/superpowers/follow-ups-after-4.0.0-remediation.md @@ -40,22 +40,21 @@ plain major bump and correctly signals the breaking changes. That collides with existing artifacts, and the resolution needs one approval before anything destructive happens: -- The `v3.0.0` and `v3.5.0` git tags exist but correspond to no published - artifact. They must be deleted and `v3.0.0` re-created at the release commit; - leaving `v3.0.0` pointing at old code while publishing a *different* 3.0.0 would - be worse than deleting it. Deleting pushed tags is outward-facing, so it waits - for an explicit go-ahead. -- The CHANGELOG's `[3.0.0] - 2026-01-02` and `[3.5.0] - 2026-06-13` sections - describe development milestones that never shipped. Recommendation: relabel them - as never-published rather than delete them, then retitle the unreleased `[4.0.0]` - section `[3.0.0]`. Preserves the record, removes the collision, tells the truth. -- **The CHANGELOG and the published record diverge in both directions**, found - while checking the above: the CHANGELOG has a `[2.2.10]` that was never - published, and is missing `2.3.0` and `2.3.1`, which were. The last CHANGELOG - entry corresponding to a real release is `2.2.9`. Worth fixing in the same pass. - -`pyproject.toml` still reads `3.5.0` and moves to `3.0.0` as part of the release -step. +- **DONE 2026-08-03.** The `v3.0.0` and `v3.5.0` tags corresponded to no + published artifact and are deleted, locally and on the remote. `v3.0.0` is + re-created at the release commit when the release is cut. The five `v2.*` tags + are kept: each matches a real PyPI or conda-forge release. +- **DONE 2026-08-03.** The two never-shipped sections are relabelled + `[3.5.0-dev]` and `[3.0.0-dev]`, each marked "never published" with a note + saying so, and the unreleased section is retitled `[3.0.0]`. Content preserved; + the collision is gone. +- **DONE 2026-08-03.** `[2.3.0]` and `[2.3.1]` are added with their real PyPI + upload dates (2024-08-02 and 2024-08-13, read from the index rather than + guessed), and `[2.2.10]` is marked never published. `[2.2.9]`'s hand-written date + disagrees with PyPI's record by a month; that is noted in place rather than + overwritten. + +`pyproject.toml` now reads `3.0.0`. **Order:** correctness leftovers -> the *subset* of test-quality findings covering files the architecture work will move -> dead-code deletion -> one model contract @@ -79,6 +78,41 @@ generally, the manifests' line numbers are stale (`ASE/thermo.py` has shifted ~500 lines), so every finding is re-verified against source before being worked, not read out of a ledger. Chemistry M4 was already fixed and no ledger knew. +## Two planning corrections, 2026-08-03 + +**The order in the plan above is wrong, and I wrote it that way.** It runs +`A -> targeted test hardening -> B2`, while B2's own stated rationale is that +deleting dead code early shrinks the test-quality cluster *before* effort goes +into it. The selection rule even says "findings naming a test whose `src/` target +B2 deletes are not hardened at all". Both point the same way: **B2 must come +before the test hardening**, and arguably before the rest of A. Corrected order: + + B2 -> remaining A -> targeted test hardening -> B1 -> B3 -> B4 -> B5 -> G -> C -> D -> F + +**That mis-ordering already cost work.** Cluster A's fallbacks-M2 fix hardened the +diagnostics in `isomers/parallel_embed.py` — a module M53 lists for deletion. +Re-verified: `use_parallel_embedding` is a constructor parameter of the isomer +engine defaulting to `False`, with no plumbing from `Auto3DOptions` or the CLI, so +no production path enables it and M53's "test-only" claim stands. The M2 fix is +correct but applies to a path no run takes. **Whether to delete the module is a +feature removal, not a dead-code cleanup, and needs a decision** — it is a public +constructor argument, so removing it changes a documented API. + +**M53's inventory is partly stale — do not delete from it without re-checking.** +Verified against current source: + +| M53 entry | status now | +|---|---| +| `STANDARD_PRESSURE` unused | **wrong** — read 4x in `ASE/thermo.py` since 2026-08-03 | +| `ASE/thermo.py` `mol2atoms` dead | **wrong** — 2 callers in `src/`; `vib_hessian` uses it | +| `constants.py` `check_connectivity` hardcodes 1.25/1.1 | recheck; the surrounding code has moved | +| `isomers/parallel_embed.py` 138 lines | claim stands, but see the decision needed above | + +The remaining nine entries were not re-checked and must be before anything is +deleted. This is the third time a manifest entry has turned out to be already +closed or wrong; the pattern is now reliable enough to treat every entry as a +claim rather than a fact. + ## What the remediation closed All 14 Criticals (C1–C14), plus M1, M2 (moot), M8–M17, M19, M21–M23, diff --git a/pyproject.toml b/pyproject.toml index 90de1216..f8c8d87b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "Auto3D" -version = "3.5.0" +version = "3.0.0" description = "Generating Low-energy 3D Conformers from SMILES/SDF" readme = "README.md" license = {text = "MIT"} diff --git a/src/Auto3D/ASE/geometry.py b/src/Auto3D/ASE/geometry.py index cd8fbbcd..41b78a21 100644 --- a/src/Auto3D/ASE/geometry.py +++ b/src/Auto3D/ASE/geometry.py @@ -164,9 +164,16 @@ def opt_geometry( opt_steps: Maximum optimization steps per structure. Defaults to 2000. patience: Drop conformer if force doesn't decrease for this many consecutive steps. Defaults to None (uses opt_steps value). - batchsize_atoms: Number of atoms per optimization batch. Larger values - use more GPU memory but may be faster. Defaults to 1024. - Recommendation: ~1024 per GB of GPU memory. + batchsize_atoms: Number of atoms per optimization batch, used **as + given**. Larger values use more GPU memory but may be faster. + Defaults to 1024. + + Note the difference from ``main()``/``Auto3DOptions``, where the same + parameter name is a per-gigabyte *multiplier*: ``ChunkManager`` + multiplies it by the detected GPU memory, so ``batchsize_atoms=1024`` + means 1024 there and 81,920 on an 80 GB card, while here it always + means 1024. Two meanings for one name, which is why each is spelled + out rather than cross-referenced. use_gpu: Use the GPU when available. Defaults to True. allow_tf32: Enable TF32 matmul precision on Ampere+ GPUs. Defaults to False. out_path: Output SDF path. Defaults to ``__opt.sdf`` diff --git a/src/Auto3D/ASE/thermo.py b/src/Auto3D/ASE/thermo.py index a0803dc7..0dda7f94 100644 --- a/src/Auto3D/ASE/thermo.py +++ b/src/Auto3D/ASE/thermo.py @@ -337,6 +337,31 @@ def _resolve_multiplicity(mol: Chem.Mol) -> int: return value n_radical = sum(a.GetNumRadicalElectrons() for a in mol.GetAtoms()) multiplicity = n_radical + 1 + # The derived value gets the same parity check the supplied one gets. + # Both the bounds and parity checks above sit inside the `HasProp` branch, so + # a multiplicity Auto3D derived itself was returned unchecked -- and 2S+1 + # requires odd multiplicity for an even-electron species and even for an + # odd-electron one, which the radical count can violate when the drawing is + # wrong (a valence-satisfied structure hiding an open shell). Getting it wrong + # is worth R*ln 3 = 0.65 kcal/mol in T*S_elec. + # + # Warn-only, and deliberately so: unlike the supplied-value branch, there is + # no further fallback to take -- this IS the fallback. Silently substituting a + # parity-consistent guess would replace a wrong number the user can see with a + # wrong number they cannot. + n_electrons = _electron_count(mol) + if multiplicity % 2 == n_electrons % 2: + logger.warning( + "Molecule %s: the multiplicity derived from its radical-electron " + "count (%d) has a parity inconsistent with a %d-electron species " + "(2S+1 requires odd multiplicity for an even-electron species, even " + "for an odd-electron one). The drawing may hide an open shell. Set " + "the 'multiplicity' property explicitly; the electronic entropy term " + "is otherwise wrong by up to R*ln(3) = 0.65 kcal/mol in T*S.", + mol.GetProp("_Name") if mol.HasProp("_Name") else "molecule", + multiplicity, + n_electrons, + ) mol.SetUnsignedProp("multiplicity", int(multiplicity)) if n_radical > 0: logger.warning( diff --git a/src/Auto3D/config.py b/src/Auto3D/config.py index 3a25709f..aa19464a 100644 --- a/src/Auto3D/config.py +++ b/src/Auto3D/config.py @@ -109,6 +109,18 @@ def check_field_bounds(values: dict) -> None: value = values[name] if name in SENTINEL_FIELDS and (value is None or value is False): continue + # `k=True` used to pass every gate and mean k=1: bool is a subclass of + # int, so operator.ge(True, 1) is True, and `top_k`'s `if k == 1` then + # matched. Harmless in effect, but `k: int | bool = False` advertises a + # bool where only `False` was ever meant as a sentinel, so `True` is a + # value the type says is legal and nothing gives a meaning to. Rejected + # rather than silently reinterpreted -- a caller who wrote it meant + # something, and it was not "one conformer". + if value is True: + raise ConfigurationError( + f"{name} must be a number, got True. Only False is a sentinel " + f"here (meaning 'not specified'); write {name}=1 for one." + ) cmp, symbol = _BOUND_OPS[kind] try: in_bounds = cmp(value, limit) @@ -286,7 +298,16 @@ class Auto3DOptions: """RAM size assigned to Auto3D in GB. None for automatic detection.""" batchsize_atoms: int = DEFAULT_BATCHSIZE_ATOMS - """Number of atoms per optimization batch per GB.""" + """Atoms per optimization batch, **per gigabyte** of detected GPU memory. + + ``ChunkManager`` multiplies this by the detected memory, so the default 1024 + means 1024 atoms per batch on a 1 GB card and 81,920 on an 80 GB one. + + ``ASE.geometry.opt_geometry`` takes the same parameter name **absolutely** -- + 1024 means 1024 there whatever the card. The two entry points are 80x apart on + the same value; each docstring says which it is rather than pointing at the + other. + """ # Performance options allow_tf32: bool = False diff --git a/tests/test_config.py b/tests/test_config.py index e33f36af..43a21b4f 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -314,3 +314,35 @@ def test_cpu_list_collapses_to_one(self): def test_cpu_empty_list_is_safe(self): from Auto3D.config import optimizer_worker_indices assert optimizer_worker_indices(False, []) == [0] + + +class TestSentinelsAreNotSilentlyReinterpreted: + """`k=True` passed every gate and meant `k=1`. + + bool is a subclass of int, so `operator.ge(True, 1)` is True and the bounds + check let it through; `top_k`'s `if k == 1` then matched. The effect was + harmless, but `k: int | bool = False` advertises a bool where only `False` was + ever meant as a sentinel, so `True` was a value the type called legal and + nothing gave a meaning to. A caller who wrote it meant something, and it was + not "one conformer". + """ + + def test_k_true_is_rejected_rather_than_read_as_one(self): + from Auto3D.config import Auto3DOptions + from Auto3D.exceptions import ConfigurationError + + with pytest.raises(ConfigurationError, match="got True"): + Auto3DOptions(path="in.smi", k=True) + + def test_k_false_is_still_the_not_specified_sentinel(self): + """False must keep working: it is how "use window instead" is spelled.""" + from Auto3D.config import Auto3DOptions + + options = Auto3DOptions(path="in.smi", k=False, window=2.0) + assert options.k is False + assert options.window == 2.0 + + def test_a_real_k_is_unaffected(self): + from Auto3D.config import Auto3DOptions + + assert Auto3DOptions(path="in.smi", k=1).k == 1 diff --git a/tests/test_packaging_metadata.py b/tests/test_packaging_metadata.py index 2ce23552..f9fcae7b 100644 --- a/tests/test_packaging_metadata.py +++ b/tests/test_packaging_metadata.py @@ -24,8 +24,30 @@ def test_python_floor_is_3_11(): assert _pyproject()["project"]["requires-python"] == ">=3.11" -def test_version_is_3_5(): - assert _pyproject()["project"]["version"].startswith("3.5") +def test_version_matches_the_newest_changelog_section(): + """``pyproject.toml``'s version must equal the newest CHANGELOG heading. + + This asserted ``startswith("3.5")`` and so had to be edited by hand on every + version change -- and was missed on one, turning three CI jobs red for a + release-prep commit whose own message claimed a green suite. Deriving the + expected value from the CHANGELOG makes the two unable to drift, and makes the + test say what it actually cares about: that the file recording the release and + the file declaring it agree. + """ + import re + + changelog = (ROOT / "CHANGELOG.md").read_text() + # The newest release heading. `-dev` sections are development records for + # versions that were never published (see CHANGELOG.md) and are skipped, or + # this would compare against a milestone rather than the shipping version. + newest = next( + m.group(1) + for m in re.finditer(r"^## \[([0-9][0-9.]*)\]", changelog, re.MULTILINE) + ) + assert _pyproject()["project"]["version"] == newest, ( + f"pyproject.toml declares {_pyproject()['project']['version']!r} while the " + f"newest CHANGELOG section is [{newest}]" + ) def test_no_jpt_package_data(): diff --git a/tests/test_thermo_helpers.py b/tests/test_thermo_helpers.py index d0a2c09d..9781ad58 100644 --- a/tests/test_thermo_helpers.py +++ b/tests/test_thermo_helpers.py @@ -1620,3 +1620,56 @@ def calculator(self): # pragma: no cover - reaching this is the bug ) assert thermo_mod._load_hessian_model(spelling, "cpu") == "the-ani-module" + + +class TestDerivedMultiplicityIsAlsoChecked: + """The bounds and parity checks only ever ran on a *supplied* multiplicity. + + Both sit inside ``if mol.HasProp("multiplicity")``, so a multiplicity Auto3D + derived from the radical-electron count was returned unchecked -- and 2S+1 + requires odd multiplicity for an even-electron species, even for an odd-electron + one, which the radical count can violate when the drawing hides an open shell. + Getting it wrong is worth R*ln 3 = 0.65 kcal/mol in T*S_elec. + + RDKit re-derives radical electrons on sanitization and is self-consistent for + every ordinary species (checked: CH2, CH3, O2, N, ethanol), which is why this + needed a deliberately inconsistent molecule rather than a realistic one -- + and why the check is worth having: the case it catches is precisely the one no + ordinary input produces. + """ + + def test_a_parity_inconsistent_derived_multiplicity_warns(self, caplog): + from rdkit import Chem + + from Auto3D.ASE import thermo as thermo_mod + + # CH3 has 9 electrons (odd), so 2S+1 must be even. Forcing two unpaired + # electrons makes the derived multiplicity 3 -- odd, and impossible. + mol = Chem.MolFromSmiles("[CH3]") + mol.GetAtomWithIdx(0).SetNumRadicalElectrons(2) + mol.SetProp("_Name", "impossible_radical") + assert thermo_mod._electron_count(mol) % 2 == 1, "test premise: odd electrons" + + with caplog.at_level(logging.WARNING, logger="Auto3D.ASE.thermo"): + multiplicity = thermo_mod._resolve_multiplicity(mol) + + assert multiplicity == 3, "the derived value is still returned, not replaced" + assert any("parity" in r.message for r in caplog.records), ( + f"an impossible derived multiplicity was returned with no warning: " + f"{[r.message for r in caplog.records]}" + ) + + def test_a_consistent_derived_multiplicity_is_not_warned_about(self, caplog): + """Every ordinary radical must stay quiet, or the check is noise.""" + from rdkit import Chem + + from Auto3D.ASE import thermo as thermo_mod + + for smiles in ("[CH3]", "[CH2]", "CCO", "[N]"): + caplog.clear() + mol = Chem.MolFromSmiles(smiles) + with caplog.at_level(logging.WARNING, logger="Auto3D.ASE.thermo"): + thermo_mod._resolve_multiplicity(mol) + assert not any("parity" in r.message for r in caplog.records), ( + f"{smiles} triggered a parity warning it should not have" + )