fix(inputs,algorithms,connect): deprecation aliases, regression fits, CSR guard, dtype#853
Merged
Merged
Conversation
… CSR guard, dtype/off-by-one - spike_current/ramp_current deprecation aliases forwarded to constant_input with wrong kwargs (crash); forward to spike_input/ramp_input (Critical) - LogisticRegression.call crashed (flattened targets then read .shape[1]); init a 1-D parameter vector (Critical) - ElasticNetRegression trained with default add_bias=True while predict used self.add_bias -> feature mismatch crash; pass add_bias=self.add_bias (High) - CSRConn.build_csr guard "pre_num != pre_num" was a tautology; compare against inptr.size-1 and raise on inconsistent pre_size (High) - connect.coo2csr scattered int counts into a uint32 buffer (int32->uint32 warning, future JAX error); use get_idx_type() (Medium) - polynomial_features allocated one extra always-zero column (off-by-one) (Medium) Findings recorded in docs/issues-found-20260619-small-modules.md
Reviewer's GuideFixes several correctness issues in small BrainPy modules: deprecation aliases for spike/ramp currents now call the correct implementations, logistic and elastic-net regressions now construct parameters/features consistently, CSR connections validate pre_size and avoid JAX dtype warnings, and polynomial feature expansion no longer allocates an extra dead column; tests and an issues-found doc were added/updated accordingly. Flow diagram for CSR connectivity validation and dtype-safe COO→CSR conversionflowchart TD
A[CSRConn.build_csr] --> B{pre_num == inptr.size - 1?}
B -- no --> C[raise ConnectorError]
B -- yes --> D[proceed to build CSR]
E[coo2csr] --> F[get_idx_type]
F --> G[allocate final_pre_count with get_idx_type]
G --> H[scatter pre_count into final_pre_count without dtype mismatch]
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 left some high level feedback:
- In
LogisticRegression.call, the new logic always initializes a 1-D parameter vector for a single output; if multi-class or multi-output use is intended, consider handling 2D targets explicitly instead of flattening to 1D so the API doesn’t silently restrict to binary problems. - In the JAX branch of
coo2csr,indptris still converted to a NumPy array viaonp.insert; consider using a JAX-native operation (e.g.,jnp.concatenate) to keep bothindicesandindptron the same device and avoid implicit host transfers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `LogisticRegression.call`, the new logic always initializes a 1-D parameter vector for a single output; if multi-class or multi-output use is intended, consider handling 2D targets explicitly instead of flattening to 1D so the API doesn’t silently restrict to binary problems.
- In the JAX branch of `coo2csr`, `indptr` is still converted to a NumPy array via `onp.insert`; consider using a JAX-native operation (e.g., `jnp.concatenate`) to keep both `indices` and `indptr` on the same device and avoid implicit host transfers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Fresh review of
brainpy/{connect,initialize,encoding,inputs,algorithms,tools}.spike_current/ramp_currentdeprecation aliases forwarded toconstant_inputwith wrong kwargs (crash); now forward tospike_input/ramp_input.LogisticRegression.callcrashed (flattened targets then read.shape[1]); now inits a 1-D parameter vector.ElasticNetRegressiontrained withadd_bias=Truewhile predict usedself.add_bias→ feature-mismatch crash; now consistent.CSRConn.build_csrguardpre_num != pre_numwas a tautology; now validates againstinptr.size-1.connect.coo2csrscattered int counts into a uint32 buffer (int32→uint32 warning / future JAX error); now usesget_idx_type().polynomial_featuresallocated one extra always-zero column (off-by-one).Prior-audit C-23/C-24/H-46/H-47 verified already-fixed. In-scope: 509 passed. Findings:
docs/issues-found-20260619-small-modules.md.Summary by Sourcery
Fix multiple correctness issues in offline regression algorithms, sparse connectivity utilities, and deprecated current input aliases, and document the findings from the small-modules audit.
Bug Fixes:
Documentation:
Tests: