fix: SupportSTDP.stdp_update keyword typo and DSRunner deprecation warning#864
Conversation
…rning - mixin.py: the base SupportSTDP.stdp_update placeholder declared the keyword `onn_post` (typo) while every concrete override and caller uses `on_post`. Align the placeholder signature. The five overrides in dnn/linear.py keep their targeted `# type: ignore[override]` (the base stays a permissive *args/**kwargs placeholder vs. their explicit params), but the comments no longer reference the now-fixed typo. - runners.py: DSRunner.predict raised `warnings.warn(...)` for the deprecated `inputs_are_batching` argument. `warnings.warn` returns None, so `raise` produced `TypeError: exceptions must derive from BaseException` instead of a warning. Emit the UserWarning and continue, as intended. Verified: `mypy brainpy/` stays clean (432 files); STDP suites (91 tests) and the DSRunner warn-path pass.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAligns the SupportSTDP.stdp_update interface with its concrete implementations by fixing a keyword typo, and corrects DSRunner.predict’s deprecation handling to emit a warning instead of raising a TypeError. Sequence diagram for DSRunner.predict deprecation warning flowsequenceDiagram
actor User
participant DSRunner
participant warnings
User->>DSRunner: predict(inputs, inputs_are_batching)
alt inputs_are_batching is not None
DSRunner->>warnings: warn(message, UserWarning)
end
DSRunner-->>User: prediction_result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider aligning the
SupportSTDP.stdp_updatesignature with the concrete implementations (dropping*args/**kwargsand using explicit parameters) so the override relationship is LSP-compliant and the# type: ignore[override]hints can eventually be removed. - In
DSRunner.predict, you may want to set an explicitstacklevelonwarnings.warnso that the deprecation warning points at the caller rather than the internal runner implementation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider aligning the `SupportSTDP.stdp_update` signature with the concrete implementations (dropping `*args/**kwargs` and using explicit parameters) so the override relationship is LSP-compliant and the `# type: ignore[override]` hints can eventually be removed.
- In `DSRunner.predict`, you may want to set an explicit `stacklevel` on `warnings.warn` so that the deprecation warning points at the caller rather than the internal runner implementation.
## Individual Comments
### Comment 1
<location path="brainpy/runners.py" line_range="455-460" />
<code_context>
if inputs_are_batching is not None:
- raise warnings.warn( # type: ignore[misc] # latent bug: warnings.warn returns None; preserved to avoid behavior change
+ warnings.warn(
f'''
- `inputs_are_batching` is no longer supported.
+ `inputs_are_batching` is no longer supported.
The target mode of {self.target.mode} has already indicated the input should be batching.
''',
UserWarning
</code_context>
<issue_to_address>
**suggestion:** Refine the warning to give a clearer location and potentially a more specific warning type.
With the misuse of `raise warnings.warn(...)` corrected, consider setting an appropriate `stacklevel` (e.g., `stacklevel=2`) so the warning points to the caller instead of the runner internals. Since this concerns a deprecated argument, using `DeprecationWarning` or a dedicated warning subclass instead of `UserWarning` would better express the deprecation intent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| warnings.warn( | ||
| f''' | ||
| `inputs_are_batching` is no longer supported. | ||
| `inputs_are_batching` is no longer supported. | ||
| The target mode of {self.target.mode} has already indicated the input should be batching. | ||
| ''', | ||
| UserWarning |
There was a problem hiding this comment.
suggestion: Refine the warning to give a clearer location and potentially a more specific warning type.
With the misuse of raise warnings.warn(...) corrected, consider setting an appropriate stacklevel (e.g., stacklevel=2) so the warning points to the caller instead of the runner internals. Since this concerns a deprecated argument, using DeprecationWarning or a dedicated warning subclass instead of UserWarning would better express the deprecation intent.
Fixes the two latent runtime bugs flagged during the package-wide mypy pass (#863).
1.
brainpy/mixin.py—onn_postkeyword typoThe base
SupportSTDP.stdp_updateplaceholder declared the keywordonn_post, while every concrete override (brainpy/dnn/linear.py) and every caller (brainpy/dyn/projections/plasticity.py, tests) useson_post. The placeholder body onlyraise NotImplementedError, so this was a contract/signature defect rather than a wrong-result bug, but it was the source of the STDP[override]friction. Signature aligned toon_post.The five overrides keep their targeted
# type: ignore[override](the base remains a permissive*args/**kwargsplaceholder vs. their explicit params, so the LSP mismatch stands), but their comments no longer reference the now-fixed typo.2.
brainpy/runners.py:455—raise warnings.warn(...)DSRunner.predictdidraise warnings.warn(...)for the deprecatedinputs_are_batchingargument.warnings.warnreturnsNone, soraise NoneproducedTypeError: exceptions must derive from BaseExceptioninstead of a warning. Now emits theUserWarningand continues, as intended.Validation
python -m mypy brainpy/→Success: no issues found in 432 source files.predict(..., inputs_are_batching=True)now warns and continues (previously raisedTypeError).SupportSTDP.stdp_updatenow acceptson_post=and still raisesNotImplementedError.boost_linear_test.py,mixin_coverage_test.py,test_dense_stdp_update_promotes_weight): 91 passed.Summary by Sourcery
Align STDP mixin and linear layer signatures and fix DSRunner deprecation handling so it emits a warning instead of raising an error.
Bug Fixes: