fix: update pixi lockfile and address Marcus's docstring nits on previous PR (i had forgotten to add my commits before merging)#184
Merged
k-chrispens merged 6 commits intomainfrom Mar 24, 2026
Conversation
k-chrispens
requested review from
Copilot and
marcuscollins
and removed request for
Copilot
Contributor
📝 WalkthroughWalkthroughMoved chiral-gradient import to module scope, expanded RF3 docstrings, made RF3Wrapper.step() raise ValueError when chiral tracking is enabled but original chiral features are missing/empty, added two RF3 CLI flags and threaded them into guidance-config population, and tightened Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as run_grid_search.py
participant Config as GuidanceConfig
participant RF3 as RF3Wrapper
participant Loss as calc_chiral_grads_flat_impl
User->>CLI: invoke with --model rf3 --track-chiral-features
CLI->>Config: populate_config_for_guidance_type(args)
Config->>RF3: pass config.track_chiral_features=True
RF3->>RF3: featurize / prepare chiral tensors
alt chiral features present
RF3->>Loss: compute chiral gradients
Loss-->>RF3: chiral gradients
RF3->>RF3: aggregate L2 norms and continue
else chiral features missing/empty
RF3-->>Config: raise ValueError (tracking enabled but features absent)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…heck so we can detect upstream changes better
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates RF3 guidance configurability and documentation, mainly adding CLI/config plumbing for RF3 chiral feature toggles and clarifying docstrings in the RF3 wrapper.
Changes:
- Pass RF3 chiral feature flags (
disable_chiral_features,track_chiral_features) from grid search CLI intoGuidanceConfigjob configs. - Clarify RF3 wrapper docstrings for defaults and accepted MSA formats.
- Refactor RF3 chiral tracking logic in
RF3Wrapper.step()(and movecalc_chiral_grads_flat_implimport to module scope).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/sampleworks/utils/guidance_script_arguments.py |
Propagates RF3 chiral feature flags from CLI args into per-job GuidanceConfig. |
src/sampleworks/models/rf3/wrapper.py |
Docstring clarifications and refactor of chiral gradient tracking computation/guard. |
run_grid_search.py |
Adds RF3-specific CLI flags and logs their values; feeds into GuidanceConfig.populate_config_for_guidance_type(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+558
to
+568
| if self._track_chiral_features: | ||
| if ( | ||
| self._original_chiral_centers is not None | ||
| and self._original_chiral_dihedral_angles is not None | ||
| and self._original_chiral_centers.shape[0] > 0 | ||
| ): | ||
| raise ValueError( | ||
| "Chiral feature tracking is enabled, but original features are missing or empty" | ||
| ". Cannot compute chiral gradients for tracking. This may be due to an upstream" | ||
| " change in RF3 or RF3Wrapper.featurize()." | ||
| ) |
There was a problem hiding this comment.
The guard for chiral feature tracking is inverted: the code currently raises when the original chiral features are present and non-empty, and then proceeds to compute chiral gradients when they are missing/empty. This will make track_chiral_features unusable (it will error on normal inputs, and may compute with None tensors otherwise). Flip the condition so the ValueError triggers only when required original features are missing (and decide whether an empty chiral_centers tensor should be treated as a skip vs. error).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
run_grid_search.py (1)
493-503: Add parse-time validation for incompatible RF3 chiral flags.
--disable-chiral-featuresand--track-chiral-featurescan currently be enabled together, which is a contradictory config and should fail fast in CLI parsing.Suggested patch
@@ - return parser.parse_args() + args = parser.parse_args() + if args.disable_chiral_features and args.track_chiral_features: + parser.error( + "--disable-chiral-features and --track-chiral-features are mutually exclusive." + ) + return args🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@run_grid_search.py` around lines 493 - 503, The two RF3 chiral flags (--disable-chiral-features and --track-chiral-features) are mutually exclusive but currently allowed together; after creating the ArgumentParser (the parser variable) and parsing arguments, add parse-time validation that checks if args.disable_chiral_features and args.track_chiral_features are both true and fail fast by calling parser.error(...) (or use a mutually_exclusive_group when defining the options) so the CLI exits with a clear message; locate the parser and the parse_args() call in run_grid_search.py to implement this check or replace the two parser.add_argument(...) calls with a parser.add_mutually_exclusive_group() and add both flags to that group.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@run_grid_search.py`:
- Around line 493-503: The two RF3 chiral flags (--disable-chiral-features and
--track-chiral-features) are mutually exclusive but currently allowed together;
after creating the ArgumentParser (the parser variable) and parsing arguments,
add parse-time validation that checks if args.disable_chiral_features and
args.track_chiral_features are both true and fail fast by calling
parser.error(...) (or use a mutually_exclusive_group when defining the options)
so the CLI exits with a clear message; locate the parser and the parse_args()
call in run_grid_search.py to implement this check or replace the two
parser.add_argument(...) calls with a parser.add_mutually_exclusive_group() and
add both flags to that group.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff79bd79-0d64-4d1d-ad87-1fbb21ac7b49
📒 Files selected for processing (3)
run_grid_search.pysrc/sampleworks/models/rf3/wrapper.pysrc/sampleworks/utils/guidance_script_arguments.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sampleworks/models/rf3/wrapper.py
k-chrispens
had a problem deploying
to
gpu-testing
GitHub Actions
Failure
k-chrispens
had a problem deploying
to
gpu-testing
GitHub Actions
Failure
marcuscollins
approved these changes
Mar 23, 2026
k-chrispens
changed the title
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 by CodeRabbit
Documentation
New Features
Bug Fix / Behavior Change
Chores