Repository navigation
Conversation
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.29% 66.23% -0.07%
==========================================
Files 122 122
Lines 41825 41825
Branches 10749 10749
==========================================
- Hits 27729 27702 -27
- Misses 11053 11069 +16
- Partials 3043 3054 +11
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
`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.
alongd
force-pushed
the
fix-rotor-top-neighborlist-skin
branch
from
September 26, 2026 19:00
f3a5256 to
ec678f8
Compare
alongd
added a commit
to alongd/ARC
that referenced
this pull request
Oct 2, 2026
…rom inventing rings around branched centres
kfir4444
added a commit
that referenced
this pull request
Oct 4, 2026
…rift (#1066) ## What Adds a `codecov.yml` (23 lines, no code change): - `coverage.status.project.default.threshold: 1%` - `coverage.status.patch.default.target: 80%` ## Why ARC has no Codecov configuration at all — `.coveragerc` configures coverage.py's *collection*, not Codecov's *status checks*. With no config, Codecov falls back to its built-in defaults, under which the `project` status is `target: auto` with a **0% threshold**: total coverage may never dip by any amount relative to the base commit. The consequence is that nearly every PR adding code covered below the repository average (~67%) reports a failing check while every test job, CodeQL and the functional suite are green. Observed on the current open set, all red on `codecov/project` alone: | PR | status | |---|---| | #653 | `67.03% (-0.10%)` → `67.05% (-0.08%)` after adding a genuine unit test | | #860, #1016, #1039, #1050, #1054, #1055, #1056, #1057 | red on `codecov/project`, everything else green | On #653, `codecov/patch` simultaneously reported *"Coverage not affected"* — the gate that actually judges whether the PR's new code is tested was already satisfied. A check that fails on one-tenth of a percentage point trains reviewers to ignore it, which costs us the patch signal too. ## What this does and does not change - **Does:** lets total coverage drift up to 1% without failing the status. - **Does:** keep an explicit `patch` target, so the coverage of a PR's own new or changed lines remains a gating signal. This is the stricter half of the config, not a loosening. - **Does not:** touch any test or source file, change `.coveragerc`, or alter what coverage is collected. - **Does not:** raise ARC's actual coverage. That is a separate and much larger piece of work. Note that Codecov re-evaluates statuses on new uploads, so already-open PRs will pick this up on their next CI run rather than retroactively.
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.