Conversation
`rotor_top()` decides which atoms rotate across a pivot bond, and rejects the
rotor outright if the two pivots remain connected after the bond is cut -- a
ring. It builds that graph with `build_neighbor_list()`, whose `skin` defaults
to 0.3 Angstrom per atom. That buffer is added to the neighbour test itself, so
a pair counted as bonded at
d < mult * sum_of_covalent_radii + 0.6
rather than the `d < mult * sum_r` the function reasons in throughout, and which
the pivot-distance check ten lines below applies literally. The graph and the
check it feeds were on two different cutoffs.
0.6 Angstrom is enough to promote 1-3 pairs to bonds. On 1,1-dimethylhydrazine
the three heavy atoms around the central nitrogen sit 2.33-2.38 Angstrom apart
against radii sums of 1.47-1.52, so every one of them fused, C-N-C closed into a
triangle, and all three rotors were rejected with
The pivot bond 0-1 is part of a ring; a 1D rotor scan is ill-defined.
The molecule is acyclic. The message is a true statement about the graph and a
false one about the chemistry, which is what makes this worth guarding: it is a
plausible sentence, so on any other species the natural response is to drop the
rotor and move on -- silently losing the torsional contribution instead of
failing. Any branched heavy atom is affected, which is most of a real corpus.
Real bonds here sit at d/sum_r <= 1.03 against the 1.2 threshold, so the
tolerance was never the tight part; skin=0.0 restores the intended semantics and
leaves the stretched-TS allowance (`pivot_mult`) untouched.
The existing ethane test cannot catch this class at all: with no branched centre
it has no heavy-atom 1-3 pair. Added a test on the real geometry that does.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The cutoff fix and regression coverage address the false ring detection without unresolved issues.
Pull request overview
This PR prevents false ring detection in branched molecules by removing ASE’s unintended neighbor-list skin buffer.
Changes:
- Sets
skin=0.0inrotor_top(). - Adds dimethylhydrazine regression coverage.
File summaries
| File | Description |
|---|---|
arc/job/adapters/scripts/ase_script.py |
Uses exact geometric bond cutoffs. |
arc/job/adapters/ase_test.py |
Verifies branched molecules do not produce spurious rings. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1054 +/- ##
==========================================
- Coverage 66.16% 66.16% -0.01%
==========================================
Files 122 122
Lines 41824 41824
Branches 10751 10751
==========================================
- Hits 27673 27671 -2
+ Misses 11094 11093 -1
- Partials 3057 3060 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alongd
added a commit
to alongd/ARC
that referenced
this pull request
Sep 21, 2026
…rom inventing rings around branched centres
This branch has not been deployed
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.
What
rotor_top()inarc/job/adapters/scripts/ase_script.pybuilds its bond graph withbuild_neighbor_list(...)and never setsskin, so it inherits ASE's default of 0.3 Å peratom. That buffer enters the neighbour test itself, making a pair "bonded" at
instead of the
d < mult * sum_rthe function reasons in throughout — and which thepivot-distance check ten lines below applies literally. One decision, two cutoffs.
One line changes:
skin=0.0.Why it matters
0.6 Å is enough to promote 1-3 pairs to bonds. On 1,1-dimethylhydrazine (
CN(C)N) at itsoptimized geometry the three heavy atoms around the central nitrogen sit 2.33–2.38 Å apart
against covalent-radii sums of 1.47–1.52:
With the default skin all three 1-3 pairs become bonds, C–N–C closes into a triangle, and every
rotor in the molecule is rejected:
The molecule is acyclic. The message is true about the graph and false about the chemistry,
which is the part worth guarding against: it is a chemically plausible sentence, so the natural
response on any other species is to drop that rotor and carry on — silently losing the torsional
contribution rather than failing. Any branched heavy atom is exposed, which is most of a real
corpus.
Real bonds here are at d/Σr ≤ 1.03 against the 1.2 threshold, so the tolerance was never the tight
part.
pivot_mult, the stretched-TS allowance, is untouched.Testing
arc/job/adapters/ase_test.py— 18 passed.The existing
test_rotor_topcannot catch this class: ethane has no branched centre, so it hasno heavy-atom 1-3 pair anywhere in the fixture. Added
test_rotor_top_does_not_invent_a_ring_from_1_3_neighbourson the real geometry, which failswithout the change.
Notes
Found by running a queued UMA rotor scan on a cluster, where all three rotors of the probe molecule
came back rejected. Orthogonal to #985 (the ASE queue-submission path) — that PR touches
ase_script.pyonly inapply_constraints, and leaves this line as it is onmain.