Repository navigation
Remove about 500 lines of redundant source code - #85
Merged
Merged
Conversation
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
force-pushed
the
refactor/remove-redundant-code
branch
from
November 6, 2026 15:51
6297c28 to
34dfe30
Compare
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.
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:
_SolidStateImagecandidates.pyand union-find inscan.py, now NetworkX, which is already a dependencyReview commit by commit. The tests pass at each commit, and the comparison below ran on the final tree.
ase_npt,runtime, store, status, scan,_durableLines by area
pathway,ssneb)run,restart,ase_npt,runtime,mlip)py.typed,_durable(shared helpers moved here)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, andReactionTracker.processare 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_idsgains 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_idsreads 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_regionsitself stays slower than onmain(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.mainscan, 288 atoms, 1,208 frames, default radiusscan, same frames,--radius 2scan, quieter 700 K trajectorystatuson a campaign with recorded events--help,validate,modelsWhy
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
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.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.docs/figures/make_data.py:_SolidStateImage.filtered, theconfigparameter ofsearch.labels.bond_changes, private helpers such asASECalculatorAdapter._factory,BondChangeDetector._threshold, andReactionRun._queue_pathway, and import-only attributes such ascandidates.atom_idsandcampaign.re.srcchanges, and no file is added or removed. No test is edited.Known differences
All need contrived input.
_EndpointUnresolvedinside a bandunresolvedfailed_durable.file_digeston an empty non-blocking pipe through/dev/fdTypeErrorFIREreads.positionson a private SSNEB image under pressurefailedStopIterationraised insidepublish, only by patching itStopIterationRuntimeErrorReactionTracker.processcalled within about 25 frames of itRecursionError(more stack frames)ReactionTrackerwith numpy, float, or bool IDs mixed with ints(0, 1, np.int64(2), 3), andwrite_checkpointraisesTypeError(0, 1, 2, 3), andwrite_checkpointsucceeds. Values equal, element types differtracker.region_radius = 1.5set after constructionTypeErrorregion_radiuspassed toscan_frames(2**31 was the smallest tried)scan --radiusandcandidate_region_radiusshould do the sameBondDetectorConfigsubclass whosethreshold_foris impure or returns a one-shot iterableValueErroron the second bond of that pair_factoryor_threshold_iniwith a numpy float, under numpy 2 onlynp.float64(1.5)1.5. Judging from the code alone, anEONSettingsthat bypasses validation should reach the same line; not comparedTwo more were seen only by patching module attributes in a test: an
EOFErrorfrom something other thaninput()duringinitprints "Stopped" instead of a traceback, and a patched model catalog with an undescribed task raisesKeyErrorinstead of showing a blank label.Reasoned, not shown: a race between workers on
trajectory-contract.jsoncan raise the other of its twoValueErrors, 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 twoscanrows faster (−10.0% and −8.0% againstmain, 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
mainalready has are preserved here on purpose. Their fixes belong in separate PRs.Testing
mainand on this tree and compares exit code, stdout, stderr, and every file written, after masking paths, random tokens, nonzerobarrier_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.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).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-lineSSNEBchange 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.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 touchedpathway.pyand 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.ruffis 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
srunorsbatch, 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 inscan --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 producedci_neb_failedorunresolved, andfailedandtime_limitcame 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.