feat!: close cluster A and prepare the 3.0.0 release - #133
Merged
Conversation
Finishes the last three cluster-A findings and does the version work.
m6 -- the bounds and parity checks in _resolve_multiplicity both sat inside
`if mol.HasProp("multiplicity")`, so a multiplicity Auto3D derived from the
radical-electron count was returned unchecked. 2S+1 requires odd multiplicity for
an even-electron species and even for an odd-electron one, which the radical count
can violate when a valence-satisfied drawing hides an open shell; getting it wrong
is worth R*ln 3 = 0.65 kcal/mol in T*S_elec. Warn-only, deliberately: unlike the
supplied-value branch this IS the fallback, and substituting a parity-consistent
guess would replace a wrong number the user can see with one they cannot.
RDKit re-derives radical electrons on sanitization and is self-consistent for
every ordinary species -- checked CH2, CH3, O2, N and ethanol, none of which trip
it -- so the test forces the inconsistency. That is also the argument for the
check: the case it catches is the one no ordinary input produces.
L5 -- `k=True` passed every gate and meant k=1. bool subclasses 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 a
sentinel, so True was a value the type called legal and nothing gave a meaning to.
Rejected rather than reinterpreted: a caller who wrote it meant something, and it
was not "one conformer". False keeps working.
L6 -- `batchsize_atoms` is a per-gigabyte multiplier in main()/Auto3DOptions and
absolute in opt_geometry: 1024 means 81,920 on an 80 GB card through one entry
point and 1024 through the other. Both docstrings now say which they are instead
of one pointing at the other. Documented rather than unified, because unifying
changes memory sizing for existing callers.
Release preparation, per the decision to ship 3.0.0 rather than 4.0.0 (no 3.x ever
reached PyPI or conda-forge, so 2.3.1 -> 3.0.0 is a plain major bump instead of a
gap users have to explain to themselves):
- pyproject.toml: 3.5.0 -> 3.0.0.
- The unreleased CHANGELOG section is retitled [3.0.0].
- The two never-shipped sections become [3.5.0-dev] and [3.0.0-dev], each marked
never published with a note saying so. Content preserved, collision gone.
- The v3.0.0 and v3.5.0 tags are deleted, locally and on the remote: they matched
no published artifact, and leaving v3.0.0 on old code while publishing a
different 3.0.0 is worse than removing it. The five v2.* tags are kept, each
matching a real release. v3.0.0 is re-created at the release commit.
- [2.3.0] and [2.3.1] are added -- both were published to PyPI with no CHANGELOG
entry ever written -- with their real upload dates read from the index rather
than guessed. [2.2.10] is marked never published: this file had it while PyPI
went 2.2.9 -> 2.3.0. [2.2.9]'s hand-written date disagrees with PyPI's by a
month, which is noted in place rather than overwritten.
Both behavioral fixes mutation-verified. Verified: 1289 passed, 9 skipped; ruff
clean.
Two planning errors of mine, recorded so the next session does not repeat them. The order in the plan of record runs A -> test hardening -> B2, while B2's own rationale is that deleting dead code early shrinks the test-quality cluster before effort goes into it, and the selection rule says findings whose target B2 deletes should not be hardened at all. Both point the same way: B2 belongs first. I wrote the rationale and then ordered against it. 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 an isomer-engine constructor parameter 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 fix is correct but applies to a path no run takes. Whether to delete the module is a feature removal rather than a dead-code cleanup -- it is a public constructor argument -- so it needs a decision rather than a sweep. M53's inventory is also partly stale, and two entries are now provably wrong: STANDARD_PRESSURE is read four times in ASE/thermo.py since today, and mol2atoms has two callers in src/ including vib_hessian. Both were dead when the audit was written and are not now. The remaining nine entries were not re-checked and must be before anything is deleted. Third time a manifest entry has proven already-closed or wrong. Every entry is a claim to verify, not a fact to act on.
test_version_is_3_5 asserted startswith("3.5"), so it had to be hand-edited on
every version change. I changed pyproject.toml to 3.0.0 and did not, which turned
all three fast CI jobs red on a commit whose own message claimed "1289 passed".
The claim was not a lie about a number -- it was a suite run BEFORE the release
prep, reported for a commit that also contained the release prep. Running the
tests and then adding more content to the same commit makes the verification
statement false without anything looking wrong. The fix for that is procedural,
but the test can carry its own weight: it now derives the expected version from
the newest CHANGELOG heading, so pyproject.toml and the file recording the release
cannot drift, and no future version bump needs to remember this test exists.
`-dev` headings are skipped by the pattern -- those are development records for
versions that never published -- so the comparison is against the shipping
version and not a milestone. Verified: the regex picks 3.0.0 out of
[3.0.0], [3.5.0-dev], [3.0.0-dev], [2.3.1]; and making pyproject disagree fails
the test with both values named.
Verified after every change this time: 1289 passed, 9 skipped on two seeds; ruff
clean.
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.
Finishes cluster A (plan: #130) and does the version work.
m6 — the derived multiplicity was never checked
Both the bounds and parity checks in
_resolve_multiplicitysit insideif mol.HasProp("multiplicity"), so a multiplicity Auto3D derived from the radical-electron count came back unchecked. 2S+1 requires odd multiplicity for an even-electron species and even for an odd-electron one — which the radical count can violate when a valence-satisfied drawing hides an open shell. Wrong by one step is R·ln 3 = 0.65 kcal/mol in T·Selec.Warn-only, deliberately. Unlike the supplied-value branch, this is the fallback — there is nothing further to fall back to. Substituting a parity-consistent guess would replace a wrong number the user can see with one they cannot.
RDKit re-derives radical electrons on sanitization and is self-consistent for every ordinary species — CH₂, CH₃, O₂, N and ethanol were checked and none trip it — so the test forces the inconsistency. That is also the argument for the check: the case it catches is the one no ordinary input produces.
L5 —
k=Truepassed every gate and meantk=1boolsubclassesint, sooperator.ge(True, 1)is True and the bounds check let it through;top_k'sif k == 1then matched. Harmless in effect, butk: int | bool = Falseadvertises a bool where onlyFalsewas ever meant as a sentinel, soTruewas a value the type called legal and nothing gave a meaning to. Rejected rather than reinterpreted — a caller who wrote it meant something, and it was not "one conformer".k=Falsekeeps working, since that is how "usewindowinstead" is spelled.L6 — one parameter name, two meanings 80× apart
batchsize_atomsis a per-gigabyte multiplier inmain()/Auto3DOptionsand absolute inopt_geometry: the default 1024 means 81,920 on an 80 GB card through one entry point and 1024 through the other. Both docstrings now state which they are, rather than one cross-referencing the other. Documented rather than unified, because unifying changes memory sizing for existing callers.Release preparation
Shipping 3.0.0, not 4.0.0: no 3.x ever reached PyPI or conda-forge, so
2.3.1 → 3.0.0is a plain major bump rather than a gap users have to explain to themselves.pyproject.toml:3.5.0→3.0.0.[3.0.0].[3.5.0-dev]and[3.0.0-dev], each marked never published with a note explaining it. Content preserved; the version collision is gone.v3.0.0andv3.5.0tags are deleted, locally and on the remote. They matched no published artifact, and leavingv3.0.0on old code while publishing a different 3.0.0 is worse than removing it. The fivev2.*tags are kept — each matches a real PyPI or conda-forge release.v3.0.0gets re-created at the release commit.The CHANGELOG disagreed with the package index in both directions
Found while doing the above, and fixed in the same pass:
[2.3.0]and[2.3.1]are added with their real upload dates read from the index, not guessed — I had invented plausible dates on the first attempt and replaced them once I checked.[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 silently overwritten, since someone wrote it deliberately.Verification
Truerejection fails the L5 test; dropping the parity warning fails the m6 test.kis unaffected.Cluster A is now closed. L4 (
check_connectivityhas no opinion on element-table misses) is the one item left, and it is documented-by-design rather than a defect — recommend closing it as such rather than fixing.