feat(output): add optional timed molecule output#2436
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CLI flags and ReacNetGenerator options to emit optional per-molecule timeline CSV and optional per-reaction-event CSV; implements collection helpers and chunked spool merging for molecule timelines, extends reaction processing to emit timed event rows, adds timestep normalization, tests, and documentation. ChangesMolecule-time and reaction-event output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2436 +/- ##
==========================================
- Coverage 95.22% 94.88% -0.35%
==========================================
Files 17 17
Lines 1528 1758 +230
==========================================
+ Hits 1455 1668 +213
- Misses 73 90 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Merging this PR will improve performance by 10.6%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | WallTime | test_benchmark_hmm[reacnetgen_param3] |
12.9 ms | 11.7 ms | +10.05% |
| ⚡ | WallTime | test_benchmark_hmm[reacnetgen_param1] |
2.2 ms | 2 ms | +10.7% |
| ⚡ | WallTime | test_benchmark_hmm[reacnetgen_param2] |
2.2 ms | 2 ms | +11.06% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing hcustc:feat/timed-molecule-output (f750792) with master (6092d9d)
Footnotes
-
8 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reacnetgenerator/commandline.py`:
- Around line 275-286: The current checks for presence of numeric filters use
truthiness so numeric 0 is treated as missing; change the conditions that read
if pp.get("moleculeframes", None): and if pp.get("moleculetimesteps", None): to
explicit "is not None" checks (e.g. if pp.get("moleculeframes", None) is not
None:) so that 0 and other falsy but valid values are accepted; keep the rest of
the logic that normalizes to a list (moleculeframes / moleculetimesteps) and
appends flags to commands unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 509114a2-f26c-462f-9af1-f2730f42aede
📒 Files selected for processing (6)
docs/guide/report.mdreacnetgenerator/_path.pyreacnetgenerator/_reaction.pyreacnetgenerator/commandline.pyreacnetgenerator/reacnetgen.pytests/test_reacnetgen.py
hcustc
force-pushed
the
feat/timed-molecule-output
branch
from
16ba49d to
d01d5a4
Compare
hcustc
force-pushed
the
feat/timed-molecule-output
branch
from
3bc3cdc to
fdc2507
Compare
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
This PR adds optional time-aware molecule output and an optional per-event reaction JSONL output, enabling users to track molecules and reactions across analyzed frames and original timesteps while keeping additional per-event overhead disabled by default.
Changes:
- Add optional frame/timestep columns to molecule output, plus filtering by analyzed frames or original timesteps.
- Add optional
.reactioneventJSONL output with per-event reaction metadata (frame/timestep range + atom ids). - Extend CLI and tests to cover the new options and output formats.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_reacnetgen.py |
Adds unit tests for reaction-event details, default-off behavior, and molecule-time formatting/filtering. |
reacnetgenerator/reacnetgen.py |
Adds new configuration knobs and auto-enables molecule-time output when molecule frame/timestep filters are provided. |
reacnetgenerator/commandline.py |
Introduces CLI flags for molecule-time display/filtering and reaction-event output; updates parm2cmd accordingly. |
reacnetgenerator/_reaction.py |
Implements optional reaction-event JSONL writing and enriches reaction processing to emit per-event details when enabled. |
reacnetgenerator/_path.py |
Extends molecule printing to optionally include frame/timestep columns and filter entries by selected frames/timesteps. |
docs/guide/report.md |
Documents the extended molecule file format and the new optional reaction-event JSONL output; fixes minor typos. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @staticmethod | ||
| def _gettimestepvalue(timestep): | ||
| if isinstance(timestep, tuple): | ||
| timestep = timestep[-1] | ||
| if isinstance(timestep, np.generic): | ||
| return timestep.item() | ||
| return timestep |
| if isinstance(timestep, tuple): | ||
| timestep = timestep[-1] | ||
| if isinstance(timestep, np.generic): | ||
| return timestep.item() | ||
| return timestep | ||
|
|
|
Superseded by inline review comment: #2436 (comment) — OpenClaw 2026.4.22 (model: gpt-5.5) |
| commands.extend((f"--{ii}", str(pp[ii]))) | ||
| if pp.get("printmoleculetime", False): | ||
| commands.append("--show-molecule-time") | ||
| if pp.get("moleculeframes", None): |
There was a problem hiding this comment.
Agreed — this is a concrete correctness issue. parm2cmd() should use explicit is not None checks here so scalar 0 is preserved for valid frame/timestep filters. A small regression test for moleculeframes=0 / moleculetimesteps=0 would also be useful.
— OpenClaw 2026.4.22 (model: gpt-5.5)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: HuangChen <121350288+hcustc@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reacnetgenerator/_path.py`:
- Around line 325-337: The _shouldprintmolecule method currently treats
moleculeframes and moleculetimesteps independently causing false positives;
change it to match (frame, timestep) pairs when both filters are set: in
_shouldprintmolecule, if both self.moleculeframes and self.moleculetimesteps are
non-None, ensure timesteps is populated (call self._getmoleculetimesteps(frames)
if needed) and iterate frames with their corresponding timesteps (zip frames and
timesteps) returning True only if any (int(frame) in self.moleculeframes and
int(timestep) in self.moleculetimesteps); keep the existing behavior when only
one of the filters is set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fbe8b4e-ddd2-477e-b213-05fd37c8b8e3
📒 Files selected for processing (6)
docs/guide/report.mdreacnetgenerator/_path.pyreacnetgenerator/_reaction.pyreacnetgenerator/commandline.pyreacnetgenerator/reacnetgen.pytests/test_reacnetgen.py
✅ Files skipped from review due to trivial changes (1)
- docs/guide/report.md
🚧 Files skipped from review as they are similar to previous changes (4)
- reacnetgenerator/commandline.py
- reacnetgenerator/_reaction.py
- reacnetgenerator/reacnetgen.py
- tests/test_reacnetgen.py
| commands.append("--molecule-frame") | ||
| moleculeframes = pp["moleculeframes"] | ||
| if not isinstance(moleculeframes, (list, tuple)): | ||
| moleculeframes = [moleculeframes] | ||
| commands.extend(str(x) for x in moleculeframes) | ||
| if pp.get("moleculetimesteps") is not None: | ||
| commands.append("--molecule-timestep") | ||
| moleculetimesteps = pp["moleculetimesteps"] | ||
| if not isinstance(moleculetimesteps, (list, tuple)): | ||
| moleculetimesteps = [moleculetimesteps] | ||
| commands.extend(str(x) for x in moleculetimesteps) |
| for kk in ("moleculeframes", "moleculetimesteps"): | ||
| if kwargs[kk] is not None: | ||
| values = ( | ||
| list(kwargs[kk]) | ||
| if isinstance(kwargs[kk], (list, tuple, np.ndarray)) | ||
| else [kwargs[kk]] | ||
| ) | ||
| kwargs[kk] = [int(x) for x in values] | ||
| if ( | ||
| kwargs["moleculeframes"] is not None | ||
| or kwargs["moleculetimesteps"] is not None | ||
| ): | ||
| kwargs["printmoleculetime"] = True |
| # reaction with SMILES name like A+B->C+D | ||
| return [self._filterspec(reaction) for reaction in new_networks] | ||
| events = [] | ||
| assert stepidx is not None |
| def _shouldprintmolecule(self, frames, timesteps=None): | ||
| if self.moleculeframes is None and self.moleculetimesteps is None: | ||
| return True | ||
| assert frames is not None | ||
| if self.moleculeframes is not None: | ||
| if set(map(int, frames)).intersection(self.moleculeframes): | ||
| return True | ||
| if self.moleculetimesteps is not None: | ||
| if timesteps is None: | ||
| timesteps = self._getmoleculetimesteps(frames) | ||
| if set(timesteps).intersection(self.moleculetimesteps): |
| """Normalize stored timestep metadata to the timestep value.""" | ||
| if isinstance(timestep, tuple): | ||
| timestep = timestep[-1] | ||
| if isinstance(timestep, np.generic): |
| def test_parm2cmd_preserves_zero_molecule_filters(self): | ||
| """Frame and timestep 0 are valid molecule filters.""" | ||
| cmd = parm2cmd( | ||
| { | ||
| "inputfilename": "dummy", | ||
| "inputfiletype": "lammpsbondfile", | ||
| "atomname": ["H", "O"], | ||
| "moleculeframes": 0, | ||
| "moleculetimesteps": 0, | ||
| } | ||
| ) |
There was a problem hiding this comment.
@copilot apply changes based on this feedback
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
I re-reviewed the latest updates. The main correctness issue I still see is the unresolved combined frame/timestep filtering behavior in _shouldprintmolecule(): when both filters are set, it should match them on the same occurrence rather than as independent OR checks.
The empty-list filter handling comments are also worth addressing before merge, because parm2cmd() can currently emit --molecule-frame / --molecule-timestep without values, and ReacNetGenerator(..., moleculeframes=[]) is treated as an active filter.
CI is green and I do not see additional blockers beyond the existing inline comments.
— OpenClaw 2026.4.22 (model: gpt-5.5)
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
Re-reviewed latest head 9fa09c6.
The previous correctness blockers look addressed now:
- scalar
0frame/timestep filters are preserved inparm2cmd(); - empty frame/timestep filter lists are normalized/skipped instead of producing value-less CLI flags or active empty filters;
- combined frame+timestep filtering now matches the same
(frame, timestep)occurrence; - tuple/NumPy-array filter inputs are normalized;
- timestep normalization has been centralized in
get_timestep_value().
CI is green and the PR is mergeable. I do not see a remaining merge blocker. The only things still worth considering are non-blocking polish items already noted inline, such as replacing the assert stepidx is not None in reaction-event generation with an explicit exception and optionally handling 0-d NumPy arrays in get_timestep_value().
— OpenClaw 2026.4.22 (model: gpt-5.5)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Convert list comprehension to explicit loop for better static analysis. Each file is now immediately registered with ExitStack for guaranteed cleanup, even if an exception occurs during iteration.
Signed-off-by: HuangChen <121350288+hcustc@users.noreply.github.com>
for more information, see https://pre-commit.ci
| with ExitStack() as stack: | ||
| readers = [] | ||
| for path in paths: | ||
| readers.append(csv.reader(stack.enter_context(open(path, newline="")))) |
This PR adds optional time-aware molecule output for tracking molecule structures across analyzed frames.
Changes include:
Tests
Summary by CodeRabbit
New Features
Documentation
Tests