Repository navigation
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The ordering regression test does not cover multiple newly discovered edges within a vertex.
Review effort: Lite
Findings: None
What changed in this PR
This pull request makes Graph.get_all_edges() deterministic by preserving graph and edge insertion order while retaining deduplication.
Changes:
- Collect edges in traversal order with a separate
seenset. - Add regression tests for ordering and uniqueness.
| File | Summary |
|---|---|
arc/molecule/graph.pyx |
Implements deterministic, deduplicated edge ordering. |
arc/molecule/graph_test.py |
Adds ordering and uniqueness coverage; the fixture should also cover multiple newly discovered edges within one vertex. |
💡 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 #1056 +/- ##
==========================================
- Coverage 67.18% 67.07% -0.11%
==========================================
Files 123 123
Lines 43674 43674
Branches 11153 11153
==========================================
- Hits 29342 29294 -48
- Misses 11194 11225 +31
- Partials 3138 3155 +17
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
alongd
added a commit
to alongd/ARC
that referenced
this pull request
Sep 21, 2026
…order a function of the graph, not of hash values
alongd
added a commit
to alongd/ARC
that referenced
this pull request
Oct 2, 2026
…order a function of the graph, not of hash values
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.
… values get_all_edges() accumulated the edges into a set and returned list(edge_set). A set has no order: its iteration sequence follows the hash values of the objects it holds, so the same graph could yield its edges in a different sequence from one interpreter run to the next. That is invisible to a caller that only counts edges, and load-bearing for one that acts on the first edge it sees. arc.species.perceive.generate_lewis_structure is the latter: it feeds this list straight into an A* over bond-order assignments, so for a geometry admitting several equally valid Lewis structures it returned whichever the search happened to reach first. Perceiving one nitrous-acid geometry gave [O-][O+]=N under PYTHONHASHSEED=0 and [NH-][O+]=O under 7, 999 and 12345 -- both correct resonance forms of the same H-N-O-O connectivity, but which one you got was not a function of the input. Anything downstream that recorded the perceived species was therefore not reproducible from its own inputs. The fix preserves insertion order -- vertices in self.vertices order, and within a vertex its own edges order -- while keeping the deduplication. Set membership is order-independent, so a set is still right for that half. Verified by rebuilding the extension and perceiving that geometry under five hash seeds: identical result, where before it split. The new test fails against the old implementation under all eight seeds tried, so it detects the defect rather than merely passing. arc/molecule and arc/species are green: 953 passed, 197 subtests. Note for reviewers: rmgpy/molecule/graph.pyx carries the identical construct and is untouched here.
alongd
force-pushed
the
i066-deterministic-edge-order
branch
from
October 17, 2026 14:00
75d2419 to
bd6cf75
Compare
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.
The defect
Graph.get_all_edges()accumulated the edges into asetand returnedlist(edge_set). A set has no order — its iteration sequence follows the hash values of the objects it holds — so the same graph could yield its edges in a different sequence from one interpreter run to the next.That is invisible to a caller that only counts edges, and load-bearing for one that acts on the first edge it sees.
arc.species.perceive.generate_lewis_structureis the latter: it feeds this list straight into an A* over bond-order assignments. For a geometry admitting several equally valid Lewis structures it therefore returned whichever the search happened to reach first. Perceiving one nitrous-acid geometry:PYTHONHASHSEED[O-][O+]=N[NH-][O+]=OBoth are correct resonance forms of the same H–N–O–O connectivity — nothing here is chemically wrong. What is wrong is that which one you get is not a function of the input, so anything downstream that records the perceived species is not reproducible from its own inputs. It was found exactly that way: two runs of the same analysis over one finished ARC project disagreed on one species, deterministically within a seed and differing across seeds.
The A* itself is not at fault — its heap key carries a monotonic counter that breaks every tie. The only unstable input was the edge order.
The fix
Preserve insertion order — vertices in
self.verticesorder, and within a vertex its ownedgesorder — while keeping the deduplication. Set membership is order-independent, so a set is still the right structure for that half of the job.Graph.verticesis documented as providing "a consistent traversal order"; this makesget_all_edges()honour it.Verification
arc/moleculeandarc/species: 953 passed, 197 subtests, no regressions.One note for reviewers
rmgpy/molecule/graph.pyxcarries the identicalreturn list(edge_set)construct. It is not touched by this PR and is not on the path that surfaced this. Worth an upstream report.