Skip to content

Remove about 500 lines of redundant source code - #85

Merged
Austin243 merged 4 commits into
mainfrom
refactor/remove-redundant-code
Nov 6, 2026
Merged

Austin243 merged 4 commits into
mainfrom
refactor/remove-redundant-code

Conversation

@Austin243

@Austin243 Austin243 commented Nov 6, 2026 •

Copy link
Copy Markdown
Owner

Summary

Removes a net 503 of the 9,479 lines in src (5.3%; +730/−1,233). Code lines, without comments, docstrings, or blanks, go from 7,732 to 7,223. About 92 of the removed lines are only rewrapped: with trailing-comma layout normalized in both trees, the change is 9,362 to 8,951 lines (−4.4%), and Python tokens fall 2.8%. Commands, options, campaign files, and the documented Python API stay the same. Output files and the number and order of model evaluations matched in every comparison below, apart from the masked source digests and the resume line under Compatibility. Known differences remain on contrived input and are listed below.

What goes:

  • checks repeated after validation already ran, and branches no input reaches
  • helpers with one caller and forwarding layers, such as the forwarding methods on _SolidStateImage
  • copies of the same check: unknown keys, safe names, positive counts, charge and spin, and model file paths now share one form
  • the hand-written walk in candidates.py and union-find in scan.py, now NetworkX, which is already a dependency

Review commit by commit. The tests pass at each commit, and the comparison below ran on the final tree.

Commit Layer Lines
8d19080 detection, tracking, pathway refinement, SSNEB, restart, run, ase_npt, runtime, store, status, scan, _durable −241
227ef8c CLI, wizard, campaign loading, model setup −102
50a5cf4 model adapters −84
34dfe30 EON search −76
Lines by area
Area Before After Removed %
Pathway refinement (pathway, ssneb) 1,095 987 108 9.9
CLI, wizard, campaign, model setup 1,699 1,597 102 6.0
Adapters 1,685 1,601 84 5.0
EON search 1,846 1,770 76 4.1
Bond detection and tracking 988 916 72 7.3
MD run loop (run, restart, ase_npt, runtime, mlip) 1,119 1,073 46 4.1
Reaction store, status, scan 927 896 31 3.3
Package files including py.typed, _durable (shared helpers moved here) 121 137 −16 −13.2

There are 382 functions rather than 386, 8 rather than 10 at cyclomatic complexity 21 or above, and 2 rather than 5 at 100 lines or more. cli.main, guided_push, wizard._group, and ReactionTracker.process are shorter but barely moved in complexity (38 to 37, 29 to 29, 29 to 28, 23 to 23); splitting them is separate work. Two costs: atom_ids gains a second branch for speed, and imports of underscore names across modules rise from 8 sites to 21 because of the shared helpers.

Three small speed changes and one repair add 13 lines: atom_ids reads an integer array without checking each value, bond membership no longer builds a set per bond, thresholds are looked up once per element pair per frame, and whole-component regions are found in one pass, which undoes most of the slowdown from moving to NetworkX. _changed_regions itself stays slower than on main (0.24 to 0.47 s over 1,207 calls at the default radius). Runs with a real model are not measurably faster, because the model and imports dominate them.

CPU time

Median of 3 interleaved runs against main, Python 3.12.3, lightly loaded Mac. Two earlier passes under heavy load, on the tree before the last fixes, gave −5.6% and −7.3% for the first row and −2.4% and −2.8% for the second, so read these as ranges. Peak memory did not change in any way that repeated.

Run main This PR Change
scan, 288 atoms, 1,208 frames, default radius 9.05 s 8.23 s −9.1%
scan, same frames, --radius 2 10.23 s 9.66 s −5.6%
scan, quieter 700 K trajectory 3.26 s 3.06 s −6.1%
status on a campaign with recorded events 0.63 s 0.59 s −6.3%
--help, validate, models about 0.34 s about 0.34 s none
xTB reaction run with refinement and saddle check 3.50 s 3.53 s within noise
ANI, 200 steps 1.70 s 1.64 s within noise

Why

ReactionFlow should stay small enough to read and review. Most of the removed code re-checked values an earlier layer had already validated, or passed a call through an extra layer. The cut is modest because the Python API is documented and stays, and most of what remains is documented features.

Compatibility

  • EON campaigns in progress: the search contract hashes the source (search_code, source_sha256), so a campaign started before this PR refuses to resume after it. Finish it first, or keep it on its old install.
  • MD runs in progress: resuming across this PR works, as far as compared on short runs. On the Torch adapters (ANI-1xnr, MACE, AIMNet2, UMA) it stays exact, prints a two-line warning, and adds one line to restart-environment.jsonl, because four source digests change. The generic ASE adapter, xTB included, records no digest of ReactionFlow's source, so it prints and adds nothing. A tblite xTB restart is not exact on either side.
  • Python names gone, none named in the README, docs, examples, or docs/figures/make_data.py: _SolidStateImage.filtered, the config parameter of search.labels.bond_changes, private helpers such as ASECalculatorAdapter._factory, BondChangeDetector._threshold, and ReactionRun._queue_pathway, and import-only attributes such as candidates.atom_ids and campaign.re.
  • Nothing outside src changes, and no file is added or removed. No test is edited.

Known differences

All need contrived input.

Input Before After
Outside code raises the private _EndpointUnresolved inside a band Status unresolved Status failed
_durable.file_digest on an empty non-blocking pipe through /dev/fd TypeError Spins
A patched FIRE reads .positions on a private SSNEB image under pressure That attribute is missing, so status failed Works, because the image is now a filter subclass
A StopIteration raised inside publish, only by patching it StopIteration RuntimeError
A campaign nested exactly at the recursion limit, or ReactionTracker.process called within about 25 frames of it Works RecursionError (more stack frames)
Bonds passed by hand to ReactionTracker with numpy, float, or bool IDs mixed with ints For example (0, 1, np.int64(2), 3), and write_checkpoint raises TypeError For example (0, 1, 2, 3), and write_checkpoint succeeds. Values equal, element types differ
tracker.region_radius = 1.5 set after construction TypeError Runs
A huge region_radius passed to scan_frames (2**31 was the smallest tried) Did not return within the test's 20 s limit Returns. Judging from the code alone, scan --radius and candidate_region_radius should do the same
A BondDetectorConfig subclass whose threshold_for is impure or returns a one-shot iterable Called per bond Called per element pair per frame, so a one-shot iterable raises ValueError on the second bond of that pair
A subclass overriding private _factory or _threshold Honored Ignored
A custom Mapping that records reads of adapter options One read order Another
A direct call to private _ini with a numpy float, under numpy 2 only Writes np.float64(1.5) Writes 1.5. Judging from the code alone, an EONSettings that bypasses validation should reach the same line; not compared

Two more were seen only by patching module attributes in a test: an EOFError from something other than input() during init prints "Stopped" instead of a traceback, and a patched model catalog with an undescribed task raises KeyError instead of showing a blank label.

Reasoned, not shown: a race between workers on trajectory-contract.json can raise the other of its two ValueErrors, a publish site's recursion boundary moves by about two frames, and a selected NetworkX dispatch backend would now run inside _changed_regions.

Restoring the original _changed_regions (+20 lines) removes the mixed-ID, region_radius, process stack-depth, and huge-radius rows, as checked on a copy of an earlier tree. In one pass under load it also made the first two scan rows faster (−10.0% and −8.0% against main, rather than −7.3% and −2.8%). It costs a function of complexity 17 in place of one of 9, and that version did not go through the comparisons under Testing. This PR keeps the shorter form.

Bugs that main already has are preserved here on purpose. Their fixes belong in separate PRs.

Testing

  • Before/after comparison: a comparison suite kept outside this repository, and not part of this PR, runs 629 scripted cases on main and on this tree and compares exit code, stdout, stderr, and every file written, after masking paths, random tokens, nonzero barrier_seconds, ReactionFlow's own source digests, hostnames, traceback frames, and warning source lines. Databases are compared as their SQL dump. All are identical on Python 3.12, including 40 cases with real xTB, ANI-1xnr, MACE, AIMNet2, and UMA on the CPU. 589 of them are also identical at the oldest supported versions (ASE 3.28.0, numpy 1.26.4, NetworkX 3.4). 7 more cases with the real EON client on Linux are identical.
  • How much those cases see: they run 5,077 of the 5,085 statements on main. Of 181 planted bugs, they caught 159, the tests alone caught 4, and 18 went unnoticed by both. That run left out the real-model cases and gave each bug 40 s, so adapters ran mostly on fakes. 24 of the 159 were caught only by cases added after that bug was first missed. Five of the 18 are reachable bugs these cases can't see (candidates.py:68, :119, scan.py:183, search/eon.py:327, :394).
  • Differential testing beyond those cases: three successive rounds, each on the code as it then stood, ran about 2,640, 1,610, and 990 comparisons against main, using fuzzers and a hunk-by-hunk reading of the diff, on well over 150,000 generated inputs: API calls, wizard sessions, campaign files, damaged checkpoints, and injected faults. They found about 33, 13, and 1 real differences that the 629 cases had missed. The original behavior was restored for all of them except those listed under Known differences. The reproducer for every confirmed difference from the first two rounds was replayed before the final two-line SSNEB change and compares the same, apart from those listed. The fix from the third round had its own reproducer and the final comparison, but no further round or wider replay.
  • Model evaluations: a logger on every calculate() call compared real runs on all five backends in 193 comparisons, about 358,000 logged evaluations per tree. 69 ran before the second round's fixes, and 124 after them, before the third round's two-line change. SSNEB through the CLI and the adapter refusal tables were last compared before the second round's fixes, which touched pathway.py and three adapters. Kills and resumes ran on all five. Injected faults at evaluations ran only on xTB and ANI-1xnr, and SSNEB only on ANI-1xnr and AIMNet2. Every log matched, apart from xTB under 2 threads, which also differs between two runs of the same tree.
  • ruff is clean. 356 tests pass and 25 skip at each commit on Python 3.12. mypy, which CI does not run, goes from 111 errors to 93.

Not tested: a GPU, real srun or sbatch, the real EON client with a real model, real MACE-POLAR, named UMA models, Python 3.13 or later beyond what CI runs here, and Linux beyond the 7 EON cases. Real power loss and full disks were not tested, since faults were injected at Python calls, and two workers on one trajectory ran only with a held lock. SIGTERM was never compared during a real-model MD run or at a file operation inside an MD run. At file operations it ran only on a stand-in EON run and in scan --out, check, and the model download. Real models ran only at the newest dependency versions, and in EON mode only on a stand-in client with no kinetic step. No CLI run under MACE or UMA reached a band. No real model produced ci_neb_failed or unresolved, and failed and time_limit came only from injection. Checkpoints from earlier production runs continued only under a stand-in calculator (EMT), not under their own models. Real-model runs are short, at most 1,000 MD steps on Torch models. The comparison reads files after the last step, so what a fault leaves behind was seen only where a snapshot step was added. File-operation faults and damaged EON run files were sampled, at about 1.4% and 1% of the combinations.

Remove checks repeated after validation, branches no input reaches and
one-caller helpers in detection, tracking, pathway refinement, SSNEB,
restart, the store, status and scan. Region finding and scan's species
groups now use NetworkX in place of a hand-written walk and union-find,
and _durable gains a shared publish context manager and safe-name pattern.
Small speed changes go in atom_ids, region finding, bond membership and
per-frame thresholds.
Move the run path's error handling out of main, share one unknown-key
check and one safe-name pattern, and remove repeated guards in the
campaign loader, wizard and model setup.
Share option, path and charge-and-spin checks between the backends,
use the campaign's unknown-key check, and remove code no option reaches.
Fold the rate table's window test and the two step writers into one each,
use one stop path in the run loop, share the safe-name and unknown-key
checks, and remove code no campaign reaches.
@Austin243
Austin243 force-pushed the refactor/remove-redundant-code branch from 6297c28 to 34dfe30 Compare November 6, 2026 15:51
@Austin243
Austin243 merged commit 1950e3f into main Nov 6, 2026
5 of 8 checks passed
@Austin243
Austin243 deleted the refactor/remove-redundant-code branch November 6, 2026 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant