Repository navigation
Refactor runner module code - #248
Merged
Merged
Conversation
This branch was successfully 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.
This PR splits the long functions in the runners into smaller steps, moves the adaptive replica exchange ranking and the dynamics cache into their own modules, and fixes several bugs in the standard runner that the refactor uncovered.
Bug fixes (standard runner)
runtimewasn't a multiple of thecheckpoint_frequency, the final block ran a single integration step rather than the remaining time, since the remaining fraction of a checkpoint interval was passed to Sire as a number of steps. It also ran without GCMC moves or energy components, and appended energies that had already been saved.checkpoint_frequencyis zero.runtime,checkpoint_frequency,energy_frequency,frame_frequency,gcmc_frequencyandterminal_flip_frequencyto be multiples of thetimestep, since it runs in whole integration steps.Other fixes
rest2_scaleof the wrong length is no longer truncated.Refactoring
Runner._run(about 1,060 lines) andRepexRunner._run(about 460 lines) are split into a method per step. The standard runner's duplicated sub-block loop, checkpoint write and dynamics call now exist once.RunnerBase.__init__(about 1,050 lines) is split into logical steps, called in the same order._adaptive.py, as aWindowRankerclass and plain functions for the statistics.DynamicsCachemoved to_dynamics_cache.py, so that it can be reused, andRepexRunnernow uses it only through methods and properties. Checkpoints written before the move still load, and the viewer reads both.RepexRunner._compute_energies, a duplicate ring-break flag, and several unused variables and imports.Tests
New tests cover the final block with and without terminal flips and GCMC, move frequencies across checkpoints, energy component time stamps after a restart, the timestep constraint, restoring the adaptive data through the runner, and loading checkpoints pickled before the dynamics cache moved, by both the runner and the viewer. The full suite passes.
This PR was generated with the aid of Claude Code (Opus 5.5).