Fix neuron, training, and ODE bugs and tighten dependencies#818
Merged
chaoming0625 merged 4 commits intomasterfrom Mar 11, 2026
Merged
Fix neuron, training, and ODE bugs and tighten dependencies#818chaoming0625 merged 4 commits intomasterfrom
chaoming0625 merged 4 commits intomasterfrom
Conversation
…refactor event handling
# Conflicts: # requirements.txt
Reviewer's GuideThis PR improves error reporting and robustness across multiple modules, fixes several bugs in state handling and object updates, tightens exception handling, and updates dependency/version requirements. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Member
Author
|
@sourcery-ai title |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
set_default_odeint()you’re assigning to_DEFAULT_DDE_METHODinstead of_DEFAULT_ODE_METHOD, which will leave the ODE default unchanged and unexpectedly overwrite the DDE default. - The change in
Array.value’s setter fromself._check_tracer()toself._valuebypasses the tracer check; if_check_tracer()enforces important invariants (e.g., JAX tracing or immutability constraints), this could silently break those assumptions and should be reconsidered or replicated explicitly. - In
BackPropagation.fit, replacingjnp.mean(bm.as_jax(bm.asarray(v)))withnp.mean(np.asarray(v))forces metrics computation onto the host/CPU and may introduce unnecessary device-host transfers; consider whether keeping this on the JAX backend (or viabm.mean) would better match the rest of the code’s execution model.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `set_default_odeint()` you’re assigning to `_DEFAULT_DDE_METHOD` instead of `_DEFAULT_ODE_METHOD`, which will leave the ODE default unchanged and unexpectedly overwrite the DDE default.
- The change in `Array.value`’s setter from `self._check_tracer()` to `self._value` bypasses the tracer check; if `_check_tracer()` enforces important invariants (e.g., JAX tracing or immutability constraints), this could silently break those assumptions and should be reconsidered or replicated explicitly.
- In `BackPropagation.fit`, replacing `jnp.mean(bm.as_jax(bm.asarray(v)))` with `np.mean(np.asarray(v))` forces metrics computation onto the host/CPU and may introduce unnecessary device-host transfers; consider whether keeping this on the JAX backend (or via `bm.mean`) would better match the rest of the code’s execution model.
## Individual Comments
### Comment 1
<location path="brainpy/integrators/ode/generic.py" line_range="137" />
<code_context>
global _DEFAULT_DDE_METHOD
- _DEFAULT_ODE_METHOD = method
+ _DEFAULT_DDE_METHOD = method
</code_context>
<issue_to_address>
**issue (bug_risk):** Default ODE integrator is no longer updated; code now writes to `_DEFAULT_DDE_METHOD` twice instead of `_DEFAULT_ODE_METHOD`.
This line should continue assigning to `_DEFAULT_ODE_METHOD`; `_DEFAULT_DDE_METHOD` is already set earlier in the function.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| global _DEFAULT_DDE_METHOD | ||
| _DEFAULT_ODE_METHOD = method | ||
| _DEFAULT_DDE_METHOD = method |
There was a problem hiding this comment.
issue (bug_risk): Default ODE integrator is no longer updated; code now writes to _DEFAULT_DDE_METHOD twice instead of _DEFAULT_ODE_METHOD.
This line should continue assigning to _DEFAULT_ODE_METHOD; _DEFAULT_DDE_METHOD is already set earlier in the function.
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 Sourcery
Improve error handling, state updates, numerical methods, and dependency configuration across neuron dynamics, training, math utilities, and build metadata.
Bug Fixes:
Enhancements: