feat: Add QRH-QLSTM and CRH-QLSTM brain architectures - #73
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds QRH-QLSTM and CRH-QLSTM reservoir‑to‑LSTM brain variants: a new ReservoirLSTMBase with recurrent PPO/BPTT support, brain registrations, many example configs (quantum/classical ablations), tests, and design/spec documentation. No existing public signatures were removed. Changes
Sequence Diagram(s)mermaid Env->>Brain: observation + sensory inputs Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
chrisjz
marked this pull request as ready for review
March 12, 2026 08:24
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
packages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.py (1)
13-24: ReturnCRHBrainfrom_create_reservoir()instead ofAny.This method always constructs
CRHBrain, soAnyplusANN401drops useful type information and makes theTYPE_CHECKING/Anyescape hatch unnecessary.As per coding guidelines,
**/*.py: Implement comprehensive type annotations in all code.Also applies to: 57-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.py` around lines 13 - 24, The _create_reservoir function is currently annotated to return Any under the TYPE_CHECKING branch which loses useful type info; change the TYPE_CHECKING import so it imports the concrete CRHBrain type (or reference CRHBrain directly) instead of Any and update the _create_reservoir signature to return CRHBrain (not Any); locate the function named _create_reservoir in crhqlstm.py and adjust its return annotation and any TYPE_CHECKING/Any usage so the function returns/declares CRHBrain (using CRHBrainConfig, FeatureChannel, InputEncoding as needed) for full static typing.configs/examples/crhqlstm_thermotaxis_stationary_predators_large.yml (1)
59-63: Prefix unified sensory examples withsensory_.This config enables unified sensory modules, but the filename omits the required
sensory_prefix. Please rename it tosensory_crhqlstm_thermotaxis_stationary_predators_large.ymland update any references.As per coding guidelines,
configs/examples/*.yml: "Configuration file prefixes should useevolutionfor evolutionary optimization configs andsensoryfor unified sensory modules, placed inconfigs/examples/directory."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/examples/crhqlstm_thermotaxis_stationary_predators_large.yml` around lines 59 - 63, Rename the example config file to include the required prefix (change the filename to sensory_crhqlstm_thermotaxis_stationary_predators_large.yml) and update any references to this file (e.g., CI configs, docs, example loaders, or tests that reference crhqlstm_thermotaxis_stationary_predators_large.yml) so they point to the new sensory_ filename; ensure the file content (the sensory_modules list: food_chemotaxis, nociception, thermotaxis) remains unchanged and run any config discovery logic to verify the renamed file is picked up.packages/quantum-nematode/quantumnematode/brain/arch/qrhqlstm.py (1)
13-24: ReturnQRHBrainfrom_create_reservoir()instead ofAny.This method always constructs
QRHBrain, soAnyplusANN401drops useful type information and makes theTYPE_CHECKING/Anyescape hatch unnecessary.As per coding guidelines,
**/*.py: Implement comprehensive type annotations in all code.Also applies to: 56-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/quantum-nematode/quantumnematode/brain/arch/qrhqlstm.py` around lines 13 - 24, The _create_reservoir() function is typed to return Any under a TYPE_CHECKING guard even though it always constructs and returns a QRHBrain; change its return annotation from Any to QRHBrain (and adjust any related type hints such as QRHBrainConfig usage) so callers retain concrete type information; remove the unnecessary TYPE_CHECKING/Any escape hatch and update the other similar annotation occurrences (lines referenced 56-59) to use QRHBrain as well to satisfy the project-wide annotation guideline.configs/examples/qrhqlstm_thermotaxis_stationary_predators_large_classical.yml (1)
50-54: Prefix unified sensory examples withsensory_.This config enables unified sensory modules, but the filename omits the required
sensory_prefix. Please rename it tosensory_qrhqlstm_thermotaxis_stationary_predators_large_classical.ymland update any references.As per coding guidelines,
configs/examples/*.yml: "Configuration file prefixes should useevolutionfor evolutionary optimization configs andsensoryfor unified sensory modules, placed inconfigs/examples/directory."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/examples/qrhqlstm_thermotaxis_stationary_predators_large_classical.yml` around lines 50 - 54, The file is misnamed for unified sensory modules: rename the file from qrhqlstm_thermotaxis_stationary_predators_large_classical.yml to sensory_qrhqlstm_thermotaxis_stationary_predators_large_classical.yml and update any references to it (imports, docs, scripts, or config lists) so they point to the new name; verify the config’s sensory_modules list (food_chemotaxis, nociception, thermotaxis) remains unchanged and that any tooling that filters by prefix (e.g., loaders expecting sensory_*) will detect the renamed file.configs/examples/qrhqlstm_thermotaxis_pursuit_predators_large_classical.yml (1)
59-63: Prefix unified sensory examples withsensory_.This config enables unified sensory modules, but the filename omits the required
sensory_prefix. Please rename it tosensory_qrhqlstm_thermotaxis_pursuit_predators_large_classical.ymland update any references.As per coding guidelines,
configs/examples/*.yml: "Configuration file prefixes should useevolutionfor evolutionary optimization configs andsensoryfor unified sensory modules, placed inconfigs/examples/directory."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@configs/examples/qrhqlstm_thermotaxis_pursuit_predators_large_classical.yml` around lines 59 - 63, The config file is missing the required sensory_ prefix: rename the file to sensory_qrhqlstm_thermotaxis_pursuit_predators_large_classical.yml and update all references to that filename (imports, README examples, CI manifests, or any code loading configs) to the new name; ensure the file still contains the sensory_modules list (food_chemotaxis, nociception, thermotaxis) and that it lives under configs/examples/, complying with the "sensory" prefix guideline for unified sensory modules.packages/quantum-nematode/quantumnematode/brain/arch/_reservoir_lstm_base.py (1)
824-826: Consider adding explicitpassfor no-op methods.The method body is just a docstring, which is valid Python but slightly unusual. An explicit statement would make the intentional no-op clearer.
✨ Suggested improvement
def update_memory(self, reward: float | None = None) -> None: """No-op for reservoir-LSTM brains.""" + del reward # Unused🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/quantum-nematode/quantumnematode/brain/arch/_reservoir_lstm_base.py` around lines 824 - 826, The method update_memory currently contains only a docstring which is legal but unclear; add an explicit no-op statement by adding a `pass` (i.e., keep the docstring and add `pass` on the next line) inside the update_memory method of the reservoir-LSTM base class so the intentional no-op is explicit and readable.packages/quantum-nematode/tests/quantumnematode_tests/brain/arch/test_crhqlstm.py (2)
179-186: Strengthencopy()coverage to catch shallow copies.This only proves the copied hidden state is reset. A shallow copy of
lstm_cell,actor_head, orcriticwould still pass, so it’s worth asserting those components are distinct instances too.Example assertions
copy = brain.copy() + assert copy is not brain + assert copy.lstm_cell is not brain.lstm_cell + assert copy.actor_head is not brain.actor_head + assert copy.critic is not brain.critic assert torch.allclose(copy.h_t, torch.zeros(8)) assert torch.allclose(copy.c_t, torch.zeros(8)) assert not torch.allclose(brain.h_t, torch.zeros(8))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/quantum-nematode/tests/quantumnematode_tests/brain/arch/test_crhqlstm.py` around lines 179 - 186, The test currently only verifies hidden states are reset after calling CRHQLSTMBrain.copy(); extend it to ensure the copy is a deep copy by asserting key subcomponents are distinct instances: check that copy.lstm_cell is not brain.lstm_cell, copy.actor_head is not brain.actor_head, and copy.critic is not brain.critic (and optionally their parameters are not the same objects) in the test_copy function so shallow copies of these modules are detected.
20-64: Add-> Noneto the test methods.Most of the test functions still omit return annotations, which is out of line with the repo’s Python typing rule.
As per coding guidelines, "Implement comprehensive type annotations in all code".
Also applies to: 75-83, 121-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/quantum-nematode/tests/quantumnematode_tests/brain/arch/test_crhqlstm.py` around lines 20 - 64, The test functions (e.g., test_default_config, test_custom_config, test_invalid_lstm_hidden_dim, test_invalid_buffer_vs_chunk) are missing return type annotations; add explicit "-> None" to each test function signature to satisfy the repo typing rule, and apply the same change to the other test functions in this file mentioned in the review (those around the later blocks) so every test_* function has a "-> None" return annotation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@configs/examples/crhqlstm_pursuit_predators_small_classical.yml`:
- Around line 54-58: The example config uses unified sensory modules but the
filename lacks the required sensory_ prefix; rename the file from
crhqlstm_pursuit_predators_small_classical.yml to
sensory_crhqlstm_pursuit_predators_small_classical.yml and update any
references/imports to this config, ensuring the sensory_modules block (keys:
sensory_modules, values: food_chemotaxis, nociception) remains unchanged; follow
the repository naming guideline that unified-sensory configs in
configs/examples/ must start with sensory_.
In `@configs/examples/crhqlstm_thermotaxis_pursuit_predators_large.yml`:
- Around line 61-65: The example config uses unified sensory modules but the
filename lacks the required sensory_ prefix; rename the file from
crhqlstm_thermotaxis_pursuit_predators_large.yml to
sensory_crhqlstm_thermotaxis_pursuit_predators_large.yml and update any
references to it (tests, docs, CI, or import lists) so they point to the new
name; also confirm the config still contains the sensory_modules block
(food_chemotaxis, nociception, thermotaxis) and that no other naming conventions
(e.g., evolution_ prefix) were mistakenly applied.
In `@configs/examples/qrhqlstm_thermotaxis_pursuit_predators_large.yml`:
- Around line 58-62: The example config uses the unified sensory option
"sensory_modules" but the filename lacks the required prefix; rename the file to
start with "sensory_" (e.g.,
sensory_qrhqlstm_thermotaxis_pursuit_predators_large.yml) so it follows the
repository convention that unified sensory configs are prefixed with "sensory_"
and placed under configs/examples/, ensuring consistency with the
"sensory_modules" setting.
In `@configs/examples/qrhqlstm_thermotaxis_stationary_predators_large.yml`:
- Around line 58-62: The example config uses unified sensory modules (it
declares sensory_modules), so rename the file from
qrhqlstm_thermotaxis_stationary_predators_large.yml to
sensory_qrhqlstm_thermotaxis_stationary_predators_large.yml to match the
repository naming convention; after renaming, update any references to the old
filename (tests, README examples, index listings) so they point to
sensory_qrhqlstm_thermotaxis_stationary_predators_large.yml and ensure the file
remains located under configs/examples/.
In `@openspec/specs/qrh-qlstm-brain/spec.md`:
- Around line 92-98: Update the spec to document the short-terminal-fragment
guard: state that learn() will only trigger an episode-end PPO update when the
replay buffer contains at least bptt_chunk_length consecutive steps, and any
terminal fragment shorter than bptt_chunk_length is silently discarded (no
re-run through the LSTM and no PPO update); keep the existing behavior for
splitting the buffer into sequential bptt_chunk_length chunks and re-running
each chunk through the LSTM from its stored initial hidden state (reset hidden
state to zeros only at true episode boundaries for chunks that are processed).
- Around line 128-140: The spec incorrectly requires exact signatures for
ClassicalBrain methods; update the QRH-QLSTM brain lifecycle spec so run_brain
is declared as run_brain(params, reward=None, input_data=None, *, top_only,
top_randomize) (i.e., reward and input_data are optional) and
post_process_episode is declared as post_process_episode(*,
episode_success=None) (i.e., episode_success optional), referencing the actual
implementations in ReservoirLSTMBase / CRH-QLSTM and ensuring copy(),
prepare_episode(), learn(), and update_memory(reward) remain as specified.
---
Nitpick comments:
In `@configs/examples/crhqlstm_thermotaxis_stationary_predators_large.yml`:
- Around line 59-63: Rename the example config file to include the required
prefix (change the filename to
sensory_crhqlstm_thermotaxis_stationary_predators_large.yml) and update any
references to this file (e.g., CI configs, docs, example loaders, or tests that
reference crhqlstm_thermotaxis_stationary_predators_large.yml) so they point to
the new sensory_ filename; ensure the file content (the sensory_modules list:
food_chemotaxis, nociception, thermotaxis) remains unchanged and run any config
discovery logic to verify the renamed file is picked up.
In `@configs/examples/qrhqlstm_thermotaxis_pursuit_predators_large_classical.yml`:
- Around line 59-63: The config file is missing the required sensory_ prefix:
rename the file to
sensory_qrhqlstm_thermotaxis_pursuit_predators_large_classical.yml and update
all references to that filename (imports, README examples, CI manifests, or any
code loading configs) to the new name; ensure the file still contains the
sensory_modules list (food_chemotaxis, nociception, thermotaxis) and that it
lives under configs/examples/, complying with the "sensory" prefix guideline for
unified sensory modules.
In
`@configs/examples/qrhqlstm_thermotaxis_stationary_predators_large_classical.yml`:
- Around line 50-54: The file is misnamed for unified sensory modules: rename
the file from qrhqlstm_thermotaxis_stationary_predators_large_classical.yml to
sensory_qrhqlstm_thermotaxis_stationary_predators_large_classical.yml and update
any references to it (imports, docs, scripts, or config lists) so they point to
the new name; verify the config’s sensory_modules list (food_chemotaxis,
nociception, thermotaxis) remains unchanged and that any tooling that filters by
prefix (e.g., loaders expecting sensory_*) will detect the renamed file.
In
`@packages/quantum-nematode/quantumnematode/brain/arch/_reservoir_lstm_base.py`:
- Around line 824-826: The method update_memory currently contains only a
docstring which is legal but unclear; add an explicit no-op statement by adding
a `pass` (i.e., keep the docstring and add `pass` on the next line) inside the
update_memory method of the reservoir-LSTM base class so the intentional no-op
is explicit and readable.
In `@packages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.py`:
- Around line 13-24: The _create_reservoir function is currently annotated to
return Any under the TYPE_CHECKING branch which loses useful type info; change
the TYPE_CHECKING import so it imports the concrete CRHBrain type (or reference
CRHBrain directly) instead of Any and update the _create_reservoir signature to
return CRHBrain (not Any); locate the function named _create_reservoir in
crhqlstm.py and adjust its return annotation and any TYPE_CHECKING/Any usage so
the function returns/declares CRHBrain (using CRHBrainConfig, FeatureChannel,
InputEncoding as needed) for full static typing.
In `@packages/quantum-nematode/quantumnematode/brain/arch/qrhqlstm.py`:
- Around line 13-24: The _create_reservoir() function is typed to return Any
under a TYPE_CHECKING guard even though it always constructs and returns a
QRHBrain; change its return annotation from Any to QRHBrain (and adjust any
related type hints such as QRHBrainConfig usage) so callers retain concrete type
information; remove the unnecessary TYPE_CHECKING/Any escape hatch and update
the other similar annotation occurrences (lines referenced 56-59) to use
QRHBrain as well to satisfy the project-wide annotation guideline.
In
`@packages/quantum-nematode/tests/quantumnematode_tests/brain/arch/test_crhqlstm.py`:
- Around line 179-186: The test currently only verifies hidden states are reset
after calling CRHQLSTMBrain.copy(); extend it to ensure the copy is a deep copy
by asserting key subcomponents are distinct instances: check that copy.lstm_cell
is not brain.lstm_cell, copy.actor_head is not brain.actor_head, and copy.critic
is not brain.critic (and optionally their parameters are not the same objects)
in the test_copy function so shallow copies of these modules are detected.
- Around line 20-64: The test functions (e.g., test_default_config,
test_custom_config, test_invalid_lstm_hidden_dim, test_invalid_buffer_vs_chunk)
are missing return type annotations; add explicit "-> None" to each test
function signature to satisfy the repo typing rule, and apply the same change to
the other test functions in this file mentioned in the review (those around the
later blocks) so every test_* function has a "-> None" return annotation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a7f7eaf-e373-4e41-a1a0-35bf9cea5e1f
📒 Files selected for processing (36)
AGENTS.mdCONTRIBUTING.mdREADME.mdconfigs/examples/crhqlstm_foraging_small.ymlconfigs/examples/crhqlstm_foraging_small_classical.ymlconfigs/examples/crhqlstm_pursuit_predators_small.ymlconfigs/examples/crhqlstm_pursuit_predators_small_classical.ymlconfigs/examples/crhqlstm_thermotaxis_pursuit_predators_large.ymlconfigs/examples/crhqlstm_thermotaxis_pursuit_predators_large_classical.ymlconfigs/examples/crhqlstm_thermotaxis_stationary_predators_large.ymlconfigs/examples/crhqlstm_thermotaxis_stationary_predators_large_classical.ymlconfigs/examples/qrhqlstm_foraging_small.ymlconfigs/examples/qrhqlstm_foraging_small_classical.ymlconfigs/examples/qrhqlstm_pursuit_predators_small.ymlconfigs/examples/qrhqlstm_pursuit_predators_small_classical.ymlconfigs/examples/qrhqlstm_thermotaxis_pursuit_predators_large.ymlconfigs/examples/qrhqlstm_thermotaxis_pursuit_predators_large_classical.ymlconfigs/examples/qrhqlstm_thermotaxis_stationary_predators_large.ymlconfigs/examples/qrhqlstm_thermotaxis_stationary_predators_large_classical.ymlopenspec/changes/archive/2026-03-12-add-qrh-qlstm-variant/.openspec.yamlopenspec/changes/archive/2026-03-12-add-qrh-qlstm-variant/design.mdopenspec/changes/archive/2026-03-12-add-qrh-qlstm-variant/proposal.mdopenspec/changes/archive/2026-03-12-add-qrh-qlstm-variant/specs/crh-qlstm-brain/spec.mdopenspec/changes/archive/2026-03-12-add-qrh-qlstm-variant/specs/qrh-qlstm-brain/spec.mdopenspec/changes/archive/2026-03-12-add-qrh-qlstm-variant/tasks.mdopenspec/specs/crh-qlstm-brain/spec.mdopenspec/specs/qrh-qlstm-brain/spec.mdpackages/quantum-nematode/quantumnematode/brain/arch/__init__.pypackages/quantum-nematode/quantumnematode/brain/arch/_reservoir_lstm_base.pypackages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.pypackages/quantum-nematode/quantumnematode/brain/arch/dtypes.pypackages/quantum-nematode/quantumnematode/brain/arch/qrhqlstm.pypackages/quantum-nematode/quantumnematode/utils/brain_factory.pypackages/quantum-nematode/quantumnematode/utils/config_loader.pypackages/quantum-nematode/tests/quantumnematode_tests/brain/arch/test_crhqlstm.pypackages/quantum-nematode/tests/quantumnematode_tests/brain/arch/test_qrhqlstm.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.py (1)
35-38: Missingfeature_channelsnon-empty validator.
CRHBrainConfig(seecrh.pylines 182-189) has afield_validatorthat rejects emptyfeature_channelslists.CRHQLSTMBrainConfiglacks this validator, so an empty list would pass config construction but fail later when_compute_reservoir_feature_dimreturns 0 or when the underlyingCRHBrainvalidates.🛡️ Proposed fix to add validator
+from pydantic import Field, field_validator -from pydantic import Field ... input_encoding: InputEncoding = Field(default="linear", description="Input encoding mode.") + + `@field_validator`("feature_channels") + `@classmethod` + def validate_feature_channels(cls, v: list[FeatureChannel]) -> list[FeatureChannel]: + """Validate feature_channels is non-empty.""" + if not v: + msg = "feature_channels must be non-empty" + raise ValueError(msg) + return v🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.py` around lines 35 - 38, CRHQLSTMBrainConfig is missing the non-empty validator for feature_channels: add a Pydantic field_validator on the feature_channels field in the CRHQLSTMBrainConfig class to reject empty lists (raise ValueError with a clear message) similar to the validator in CRHBrainConfig; ensure this validator references feature_channels and aligns behavior with CRHBrainConfig so downstream methods like _compute_reservoir_feature_dim and CRHBrain validation never receive an empty list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/specs/qrh-qlstm-brain/spec.md`:
- Around line 81-84: Update the spec to reflect the actual implementation: the
critic input concatenation detaches both the reservoir features and the LSTM
hidden state, not just h_t; i.e., mirror the behavior seen in
_reservoir_lstm_base.py where the critic input is formed via
torch.cat([features.detach(), h_state.detach()]) so the spec states that both
features and h_t are detached (no gradients flow from the critic into either).
In `@packages/quantum-nematode/quantumnematode/brain/arch/qrhqlstm.py`:
- Around line 60-68: The QRHBrainConfig instantiation in _create_reservoir is
missing propagation of the shots parameter, so add shots=config.shots to the
QRHBrainConfig(...) call (referencing QRHBrainConfig and the _create_reservoir
function) so the ReservoirLSTMBaseConfig's shots value is passed through instead
of using DEFAULT_SHOTS.
---
Nitpick comments:
In `@packages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.py`:
- Around line 35-38: CRHQLSTMBrainConfig is missing the non-empty validator for
feature_channels: add a Pydantic field_validator on the feature_channels field
in the CRHQLSTMBrainConfig class to reject empty lists (raise ValueError with a
clear message) similar to the validator in CRHBrainConfig; ensure this validator
references feature_channels and aligns behavior with CRHBrainConfig so
downstream methods like _compute_reservoir_feature_dim and CRHBrain validation
never receive an empty list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ec95180-fab7-4ce5-a667-c2f9e1359436
📒 Files selected for processing (6)
AGENTS.mdopenspec/specs/qrh-qlstm-brain/spec.mdpackages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.pypackages/quantum-nematode/quantumnematode/brain/arch/qrhqlstm.pypackages/quantum-nematode/tests/quantumnematode_tests/brain/arch/test_crhqlstm.pypackages/quantum-nematode/tests/quantumnematode_tests/brain/arch/test_qrhqlstm.py
🚧 Files skipped from review as they are similar to previous changes (1)
- AGENTS.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openspec/specs/qrh-qlstm-brain/spec.md (1)
72-75:⚠️ Potential issue | 🟡 MinorDetach
reservoir_featuresin the critic requirement as well.Line 74 still says the critic consumes
[reservoir_features, h_t.detach()], which contradicts the scenario below and documents the pre-fix behavior.📝 Suggested spec fix
-The actor head SHALL receive `[reservoir_features, h_t]` as input and output action logits over 4 actions. The critic MLP SHALL receive `[reservoir_features, h_t.detach()]` as input and output a scalar value estimate. +The actor head SHALL receive `[reservoir_features, h_t]` as input and output action logits over 4 actions. The critic MLP SHALL receive `[reservoir_features.detach(), h_t.detach()]` as input and output a scalar value estimate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/specs/qrh-qlstm-brain/spec.md` around lines 72 - 75, The spec incorrectly states the critic consumes [reservoir_features, h_t.detach()]; update the requirement so the critic MLP also receives detached reservoir features (e.g., change to [reservoir_features.detach(), h_t.detach()]) so both inputs to the critic are detached, referencing the actor head and critic MLP and the symbols reservoir_features and h_t.detach() to locate the text to modify.
🧹 Nitpick comments (2)
packages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.py (2)
88-111: Avoid hard-coding the CRH channel expansion table a second time.This logic has to stay perfectly aligned with
CRHBrain’s feature extractor. If channel definitions change incrh.py, this method can silently size the LayerNorm/heads incorrectly. Prefer deriving the dimension from a shared helper or constant next to the CRH feature-construction code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.py` around lines 88 - 111, The method _compute_reservoir_feature_dim duplicates the CRH channel expansion logic and should instead call the shared computation used by the CRH feature extractor; replace the manual channel switch in _compute_reservoir_feature_dim (which references CRHQLSTMBrainConfig, config.feature_channels and config.num_reservoir_neurons) with a call to the canonical helper/constant in crh.py (e.g., the feature-dimension helper or a FEATURE_CHANNEL_EXPANSION map used by CRHBrain/CRH feature extractor) so the dim is derived from that single source of truth; import and use that helper/constant rather than re-implementing the logic here.
22-26: Convert the new class/helper docstrings to NumPy style.These docstrings are plain prose today, so the new config and override methods do not match the repo’s required docstring format.
As per coding guidelines, "Use NumPy-style docstrings for functions and classes".
Also applies to: 41-48, 52-57, 61-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.py` around lines 22 - 26, Update the plain prose docstrings in this file to NumPy-style for the class CRHQLSTMBrainConfig (which inherits ReservoirLSTMBaseConfig) and for the helper/override methods in the same module: replace free-form paragraphs with NumPy-style sections (e.g., Parameters, Returns, Raises, Attributes, Notes) that document argument names, types and defaults, return values and exceptions; ensure class-level docstring lists attributes under "Attributes" and each method (__init__, any from_dict/validate/override methods present) includes a "Parameters" section and a short one-line summary at the top to match the repo guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/specs/qrh-qlstm-brain/spec.md`:
- Around line 114-117: The spec currently uses the noun "Warmup" where the verb
phrase "warm up" is intended; update the Scenario title "Warmup + decay
schedule" and the sentence "LR SHALL warmup linearly for the specified episodes,
then decay linearly..." to use "Warm up" and "warm up" respectively so the prose
reads correctly while keeping the references to lr_warmup_episodes and
lr_decay_episodes and the behavior ("LR SHALL ... then decay linearly, with
critic LR maintaining the same ratio to actor LR") unchanged.
---
Duplicate comments:
In `@openspec/specs/qrh-qlstm-brain/spec.md`:
- Around line 72-75: The spec incorrectly states the critic consumes
[reservoir_features, h_t.detach()]; update the requirement so the critic MLP
also receives detached reservoir features (e.g., change to
[reservoir_features.detach(), h_t.detach()]) so both inputs to the critic are
detached, referencing the actor head and critic MLP and the symbols
reservoir_features and h_t.detach() to locate the text to modify.
---
Nitpick comments:
In `@packages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.py`:
- Around line 88-111: The method _compute_reservoir_feature_dim duplicates the
CRH channel expansion logic and should instead call the shared computation used
by the CRH feature extractor; replace the manual channel switch in
_compute_reservoir_feature_dim (which references CRHQLSTMBrainConfig,
config.feature_channels and config.num_reservoir_neurons) with a call to the
canonical helper/constant in crh.py (e.g., the feature-dimension helper or a
FEATURE_CHANNEL_EXPANSION map used by CRHBrain/CRH feature extractor) so the dim
is derived from that single source of truth; import and use that helper/constant
rather than re-implementing the logic here.
- Around line 22-26: Update the plain prose docstrings in this file to
NumPy-style for the class CRHQLSTMBrainConfig (which inherits
ReservoirLSTMBaseConfig) and for the helper/override methods in the same module:
replace free-form paragraphs with NumPy-style sections (e.g., Parameters,
Returns, Raises, Attributes, Notes) that document argument names, types and
defaults, return values and exceptions; ensure class-level docstring lists
attributes under "Attributes" and each method (__init__, any
from_dict/validate/override methods present) includes a "Parameters" section and
a short one-line summary at the top to match the repo guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c1be7a4-ae28-4e9e-8dd4-60935e70a0e2
📒 Files selected for processing (2)
openspec/specs/qrh-qlstm-brain/spec.mdpackages/quantum-nematode/quantumnematode/brain/arch/crhqlstm.py
This was referenced Mar 12, 2026
Merged
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.
Changes:
Summary by CodeRabbit
New Features
Documentation