fix(docs,inputs,rates): repair docs notebooks + Hz-input & RNNCell.reset(batch_size) bugs#857
Merged
Merged
Conversation
…reset(batch_size) bugs
Re-ran every notebook under docs/ in a memory-capped, sequential sandbox.
Most failures were an artifact of executing against a stale *installed*
brainpy build; the repo source already fixes them. The remainder were genuine
notebook API-drift and two real brainpy bugs.
Notebooks (re-executed green against the repo source):
- tutorial_math/array, Dedicated_Operators: wrap computed / random arrays in
bm.Array so the documented in-place assignment works (bm.random.* and Array
arithmetic now return immutable jax arrays).
- tutorial_math/control_flows: while_loop body returns a tuple matching operands.
- tutorial_math/variables: RandomState.split_keys -> split_key.
- tutorial_FAQs/gotchas_of_brainpy_transforms: tag the intentional "State as a
transform argument" gotcha raises-exception; the "this works" example now
passes a plain array instead of a State.
- tutorial_training/esn_introduction: bp.layers.Reservoir -> bp.dyn.Reservoir.
- tutorial_training/build_training_models: bp.layers.{LSTMCell,NVAR} ->
bp.dyn.*, fix the DeepRNN.update layer chain, and rewrite the SNN example to
the bp.dyn projection API (HalfProjAlignPost / Expon / CUBA / LifRef / Leaky).
- tutorial_advanced/interoperation: wrap b in bm.Array so b.value works.
brainpy fixes (with reproducing tests):
- inputs.sinusoidal_input / square_input: attach Hz (frequency) and ms (time)
units before delegating to braintools, which now rejects bare numbers.
- dyn.rates RNNCell/GRUCell/LSTMCell/ConvLSTM reset_state: accept batch_size as
an alias for batch_or_mode, matching the canonical model.reset(batch_size=...)
convention used by brainpy.dyn neurons.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @chaoming0625, your pull request is larger than the review limit of 150000 diff characters
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Summary
Ran every notebook under
docs/in a memory-capped, strictly-sequential sandbox (the task warned this is easy to crash the host), triaged failures, and fixed them.Key finding: most failures were an artifact of executing the notebooks against a stale installed
brainpybuild (the kernel's cwd is the notebook dir, soimport brainpyresolved to site-packages, not the repo). Re-running against the repo source viaPYTHONPATHclearedsimulation,training,optimizers,bp_trainingwith no code change. The remainder were genuine notebook API-drift plus two realbrainpybugs.brainpy bug fixes (with reproducing tests)
inputs.sinusoidal_input/square_input—braintoolsnow requires the frequency to carryHzunits (and, oncedtis unit-carrying, the time args to carryms); the wrappers passed bare numbers and raisedAssertionError: Frequency must be in Hz. Now attach units before delegating. Tests inbrainpy/inputs/currents_test.py.dyn.ratesRNNCell/GRUCell/LSTMCell/ConvLSTMreset_state— only understoodbatch_or_mode, so the canonicalmodel.reset(batch_size=...)convention (shared withbrainpy.dynneurons) silently reset to an unbatched shape and raisedMathError. Now acceptbatch_sizeas an alias (back-compatbatch_or_modepreserved; the existingnvarkeyword test still passes). Tests inbrainpy/dyn/rates/rnncells_test.py.Notebooks re-executed green
tutorial_math/{array,Dedicated_Operators,control_flows,variables},tutorial_toolbox/inputs,tutorial_FAQs/gotchas_of_brainpy_transforms,tutorial_training/{esn_introduction,build_training_models}— fixes range frombm.Array(...)wrapping (in-place assignment on now-immutable jax arrays),split_keys→split_key,while_looptuple carry,bp.layers.{Reservoir,LSTMCell,NVAR}→bp.dyn.*, araises-exceptiontag for an intentional gotcha, to rewriting thebuild_training_modelsSNN example onto thebp.dynprojection API (HalfProjAlignPost/Expon/CUBA/LifRef/Leaky).tutorial_advanced/interoperationgets a genuineb.valuefix (wrap inbm.Array); its MNIST-download + multi-epoch CNN/RNN training tail is CPU-heavy and not re-run end-to-end here.Left as-is (heavy / optional-dep, documented not dropped)
Given the explicit crash constraint:
integrate_flax_into_brainpy(10-epoch flax CNN times out on CPU),integrate_bp_{convlstm,lif}_into_flax(needtensorflow_datasets),dde_numerical_solvers(bp.fde.GLShortMemoryallocates a ~3 GiB array), and theclear_buffer=True/multi-device cells ofparallel_for_parameter_exploration(clear_buffer_memory()deletes the live JAX ordered-effects token reused by the next batch; the core demo works).Tests
currents_test.py,rnncells_test.py+ touched-module regression sweep (currents_coverage_test.py,rates_test.py,nvar_test.py,nvar_coverage_test.py) all pass.