Connectome rewired-null control â review nitpick follow-ups (#264) - #265
Merged
Conversation
- Expose a public `plateau_tail` wrapper in t7_continuous_ranking and import that from the control harness (drop the private `_plateau_tail` coupling). - load(): skip blank/`#`-comment lines silently but WARN on any other malformed line and on a duplicate (arm, seed) overwrite, so manifest issues are traceable. - Locate scripts/analysis/ by walking up to the repo root in the harness test (robust to nesting depth, not a hardcoded parents[5]). - Clarify the rewiring simple-graph guard: self-loops (autapses) are intentionally permitted (the real Cook connectome has 38; degree contribution is preserved). Skipped two inline findings with reasons: - config rename: the `_klinotaxis_rewired_null` order matches the committed `_ars_depletion` / `_no_respawn_control` siblings; renaming would break that consistency. - reject self-loops: the real Cook connectome has 38 chemical autapses, so a reject guard would crash the live pipeline; they are legitimate structure and degree-preserved. Verdict unchanged (DEGREE-STATISTICS re-verified); pre-commit + tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. ð âđïļ Recent review infoâïļ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ð Files selected for processing (2)
ð§ Files skipped from review as they are similar to previous changes (2)
ð WalkthroughWalkthroughAdds a public ChangesAnalysis API and manifest handling
Tests and rewiring comment
Estimated code review effort: 2 (Simple) | ~10 minutes ðĨ Pre-merge checks | â 5â Passed checks (5 passed)
âĻ Finishing Touchesð Generate docstrings
𧊠Generate unit tests (beta)
Comment |
![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
ð§đ Nitpick comments (3)
packages/quantum-nematode/tests/quantumnematode_tests/analysis/test_connectome_structure_controls.py (1)
7-12: ð Maintainability & Code Quality | ðĩ Trivial | ðĪ Low valueGood fix for the brittle hardcoded
parents[5]traversal.Walking up until
scripts/analysisis found is robust to nesting depth. One nitpick: if the directory is never found, the loop silently stops at the filesystem root andsys.pathgets a nonexistent path, so the failure surfaces later as an opaqueModuleNotFoundErroron import rather than a clear message about the missingscripts/analysisdirectory.âŧïļ Optional: fail fast with a clearer error
_root = Path(__file__).resolve() while _root != _root.parent and not (_root / "scripts" / "analysis").is_dir(): _root = _root.parent -sys.path.insert(0, str(_root / "scripts" / "analysis")) +_analysis_dir = _root / "scripts" / "analysis" +if not _analysis_dir.is_dir(): + raise RuntimeError(f"Could not locate scripts/analysis above {Path(__file__).resolve()}") +sys.path.insert(0, str(_analysis_dir))ðĪ Prompt for 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. In `@packages/quantum-nematode/tests/quantumnematode_tests/analysis/test_connectome_structure_controls.py` around lines 7 - 12, The path-walking logic in test_connectome_structure_controls.py is fine, but it can still silently fall through to the filesystem root and add a nonexistent path if scripts/analysis is missing. Update the root āĪāĨāĪ logic around the _root loop and sys.path.insert setup to fail fast with a clear error when the directory is not found, so imports donât later surface as an opaque ModuleNotFoundError.scripts/analysis/t7_continuous_ranking.py (1)
93-99: ð Maintainability & Code Quality | ðĩ Trivial | ⥠Quick winDocstring missing NumPy Parameters/Returns sections.
The new public
plateau_tailis a "Stable cross-module API" per its own docstring, but only has prose, noParameters/Returnssections.ð Suggested docstring
def plateau_tail(out_path: Path) -> tuple[float, float] | None: - """Public entry point for the plateau-tail ranked metric (see :func:`_plateau_tail`). + """Public entry point for the plateau-tail ranked metric (see :func:`_plateau_tail`). - Stable cross-module API so other analyses (e.g. the connectome-structure controls) can reuse the - exact 029 ranked metric without importing the private helper. - """ + Stable cross-module API so other analyses (e.g. the connectome-structure controls) + can reuse the exact 029 ranked metric without importing the private helper. + + Parameters + ---------- + out_path : Path + Path to the per-run ``.out`` file. + + Returns + ------- + tuple[float, float] | None + (full-clear success %, mean foods) over the final-quarter plateau tail, or + ``None`` if ``out_path`` does not exist or contains no parseable runs. + """ return _plateau_tail(out_path)As per coding guidelines, "Use NumPy-style docstrings for functions and classes."
ðĪ Prompt for 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. In `@scripts/analysis/t7_continuous_ranking.py` around lines 93 - 99, The public plateau_tail function currently has only prose in its docstring, so update the docstring on plateau_tail to use NumPy style with explicit Parameters and Returns sections. Keep the existing intent about being a stable cross-module API, and document the out_path argument plus the tuple[float, float] | None return value in the same docstring format used elsewhere in the module, such as around _plateau_tail.Source: Coding guidelines
scripts/analysis/connectome_structure_controls.py (1)
52-77: ð Maintainability & Code Quality | ðĩ Trivial | ⥠Quick winAdd test coverage for the new warning branches.
The provided test context (
test_load_parses_manifest,test_missing_file_is_none) exercises the happy path and comment-skipping, but not the newly-added malformed-line-shape warning or duplicate-(arm, seed)-overwrite warning paths introduced here. Given these are new, previously-untested behaviors central to this PR's "manifest diagnostics" goal, targeted tests would guard against regressions.ðĪ Prompt for 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. In `@scripts/analysis/connectome_structure_controls.py` around lines 52 - 77, Add targeted tests for the new warning paths in load(manifest): one test should feed a malformed manifest line that fails the expected <arm> <int seed> <out> shape and assert the âskipping malformed manifest lineâ warning is emitted, and another should include duplicate (arm, seed) entries to verify the overwrite warning is printed and the later value wins. Reuse the existing load helper and the manifest-parsing test setup so the new cases sit alongside test_load_parses_manifest and cover the new diagnostics behavior.
ðĪ 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 `@scripts/analysis/connectome_structure_controls.py`:
- Around line 60-77: The manifest parser in parse_manifest() only checks line
shape and numeric seed, so unknown arm names can be accepted and later ignored
by analyse(), causing silent sample loss. Add explicit validation that parts[0]
is one of the expected arm symbols (_WILD and _REWIRED) before storing the
entry, and emit a WARN when it is not. Keep the existing duplicate/seed
handling, but ensure typoâd arm names are rejected in the same validation path
as other malformed manifest lines.
---
Nitpick comments:
In
`@packages/quantum-nematode/tests/quantumnematode_tests/analysis/test_connectome_structure_controls.py`:
- Around line 7-12: The path-walking logic in
test_connectome_structure_controls.py is fine, but it can still silently fall
through to the filesystem root and add a nonexistent path if scripts/analysis is
missing. Update the root āĪāĨāĪ logic around the _root loop and sys.path.insert
setup to fail fast with a clear error when the directory is not found, so
imports donât later surface as an opaque ModuleNotFoundError.
In `@scripts/analysis/connectome_structure_controls.py`:
- Around line 52-77: Add targeted tests for the new warning paths in
load(manifest): one test should feed a malformed manifest line that fails the
expected <arm> <int seed> <out> shape and assert the âskipping malformed
manifest lineâ warning is emitted, and another should include duplicate (arm,
seed) entries to verify the overwrite warning is printed and the later value
wins. Reuse the existing load helper and the manifest-parsing test setup so the
new cases sit alongside test_load_parses_manifest and cover the new diagnostics
behavior.
In `@scripts/analysis/t7_continuous_ranking.py`:
- Around line 93-99: The public plateau_tail function currently has only prose
in its docstring, so update the docstring on plateau_tail to use NumPy style
with explicit Parameters and Returns sections. Keep the existing intent about
being a stable cross-module API, and document the out_path argument plus the
tuple[float, float] | None return value in the same docstring format used
elsewhere in the module, such as around _plateau_tail.
ðŠ 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ffc45ef-bfde-4093-bc73-f550580d1ad6
ð Files selected for processing (4)
packages/quantum-nematode/quantumnematode/connectome/rewiring.pypackages/quantum-nematode/tests/quantumnematode_tests/analysis/test_connectome_structure_controls.pyscripts/analysis/connectome_structure_controls.pyscripts/analysis/t7_continuous_ranking.py
Codecov Reportâ All modified and coverable lines are covered by tests. ðĒ Thoughts on this report? Let us know! |
- load(): reject unknown arm names (not in {wild_type, rewired_null}) with a
WARN instead of storing them to be silently ignored by analyse().
- Harness test: fail fast with a clear error if the scripts/analysis walk-up
can't find the directory, rather than inserting a nonexistent path and
surfacing an opaque ModuleNotFoundError.
- Add tests for the new load() diagnostics: malformed-line + unknown-arm WARNs,
and the duplicate-(arm,seed) overwrite (later value wins).
Skipped the NumPy-docstring nitpick: t7_continuous_ranking (incl. _plateau_tail)
uses prose docstrings, not NumPy Parameters/Returns; plateau_tail already matches
that convention â NumPy sections would be inconsistent with the module.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Small follow-up to #264 (merged), addressing the PR review nitpicks on the connectome rewired-null control. No behaviour change â the DEGREE-STATISTICS verdict is unaffected.
Fixed
t7_continuous_rankingnow exposes a publicplateau_tailwrapper; the control harness imports that instead of the private_plateau_tail(drops the cross-module private coupling).load()skips blank/#-comment lines silently but now WARNs on any other malformed line and on a duplicate(arm, seed)overwrite, so analysis issues stay traceable.scripts/analysis/by walking up to the repo root, not a hardcodedparents[5].Skipped (with reasons)
_klinotaxis_rewired_nullorder matches the committed siblings_ars_depletion/_no_respawn_control; renaming would break consistency with them.Gates
Full
pytest -m "not nightly"â 4090 passed; fullpre-commit run -aclean;openspec validate --specspasses.ðĪ Generated with Claude Code
Summary by CodeRabbit
(arm, seed)entries are now detected and explicitly warned before the later value overwrites the earlier one.