fix(math/object_transform): cond/ifelse, collectors, VariableView edge cases#840
Merged
Merged
Conversation
…ollectors, VariableView - bm.cond crashed on constant (non-callable) branches despite docstring allowing ArrayType/float/int/bool; wrap constants into callables (Medium) - bm.ifelse crashed (len() on bool) when `conditions` was a scalar bool/array; normalize scalar to a 1-element list (Medium) - Collector.__sub__ raised a bare KeyError instead of the descriptive ValueError used by every other not-found path (Medium) - VariableView.value setter crashed on plain inputs and never unwrapped State / canonicalized numpy dtype; mirror the hardened Variable setter (Medium) Findings recorded in docs/issues-found-20260619-math-object-transform.md
Reviewer's GuideRefines math/object_transform edge-case behavior and error handling for cond/ifelse, Collector.sub, and VariableView.value, and adds regression tests plus an issues-found audit doc. Sequence diagram for bm.cond with constant branchessequenceDiagram
actor User
participant bm_cond as bm_cond
participant _make_branch as _make_branch
participant brainstate_cond as brainstate_transform_cond
User->>bm_cond: cond(pred, true_fun, false_fun, operands)
bm_cond->>bm_cond: operands = _unwrap_state_operands(operands)
bm_cond->>_make_branch: _make_branch(true_fun)
alt branch_is_callable
_make_branch-->>bm_cond: wrapped_true_fun
else branch_is_constant
_make_branch-->>bm_cond: wrapped_constant_true_fun
end
bm_cond->>_make_branch: _make_branch(false_fun)
alt branch_is_callable
_make_branch-->>bm_cond: wrapped_false_fun
else branch_is_constant
_make_branch-->>bm_cond: wrapped_constant_false_fun
end
bm_cond->>brainstate_cond: cond(pred, wrapped_true_fun, wrapped_false_fun, operands)
brainstate_cond-->>User: result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
documentation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="brainpy/math/object_transform/controls.py" line_range="250-251" />
<code_context>
+ # it into a one-element list so it flows through the conversion below;
+ # otherwise ``brainstate.transform.ifelse`` would call ``len()`` on the
+ # scalar and raise ``TypeError: object ... has no len()``.
+ if not isinstance(conditions, (list, tuple)):
+ conditions = [conditions]
+
# Convert if-elif-else chain to mutually exclusive conditions
</code_context>
<issue_to_address>
**suggestion:** Normalising non-sequence `conditions` to a one-element list changes behaviour for other iterable types (e.g. generators); consider a more precise type check.
The current `if not isinstance(conditions, (list, tuple))` branch fixes the scalar case but also wraps generators and other non-list/tuple sequences, changing them from multiple conditions into a single one.
To keep behaviour consistent for genuine sequences, consider checking for “is a scalar/non-sequence” rather than “is not list/tuple”. For example, you could detect `collections.abc.Sequence` (excluding `str`/`bytes`/`np.ndarray`) or explicitly handle `np.bool_` and scalar arrays, so real sequences still flow through unchanged while scalars avoid the `len()` error.
Suggested implementation:
```python
branches = [make_callable(branch) for branch in branches]
# A single condition may be passed as a bare scalar bool/array (the
# docstring types ``conditions`` as ``bool, sequence of bool``). Normalise
# true scalars into a one-element list so they flow through the conversion
# below; otherwise ``brainstate.transform.ifelse`` would call ``len()`` on
# the scalar and raise ``TypeError: object ... has no len()``. Existing
# iterable conditions (lists/tuples/generators/etc.) are left unchanged so
# they still represent multiple conditions.
is_numpy_scalar_bool = False
try:
import numpy as np # use existing import if already present
if isinstance(conditions, np.bool_) or (
isinstance(conditions, np.ndarray) and conditions.ndim == 0 and conditions.dtype == bool
):
is_numpy_scalar_bool = True
except Exception:
# NumPy not available or not used; fall back to pure-Python scalars only
pass
if isinstance(conditions, bool) or is_numpy_scalar_bool:
conditions = [conditions]
# Convert if-elif-else chain to mutually exclusive conditions
if isinstance(conditions, (list, tuple)) and len(conditions) > 0:
conditions = list(conditions)
```
1. If this file does not already import NumPy as `np`, you may want to move the `import numpy as np` to the module top-level instead of keeping the local import inside the function, to match existing style/import conventions.
2. If other scalar condition types (e.g. `np.int_` or numeric scalars) should also be normalised, extend the `is_numpy_scalar_bool` logic and/or the `isinstance(conditions, bool)` check accordingly (e.g. using `numbers.Integral`), but the current change focuses on boolean scalars to match the documented type.
</issue_to_address>
### Comment 2
<location path="brainpy/math/object_transform/collectors_test.py" line_range="154-160" />
<code_context>
c - ['nope']
+def test_sub_with_list_missing_value_raises():
+ # P4-M3: removing a *value* object that is not present must raise the same
+ # descriptive ValueError as the string-key path, not a bare KeyError(id).
+ present = object()
+ absent = object()
+ c = Collector({'a': present})
+ with pytest.raises(ValueError):
+ c - [absent]
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the regression by checking the `ValueError` message content for missing values.
Since this already covers the missing-value path for `Collector.__sub__`, you can make the regression stronger by also asserting on the error message to distinguish it from a generic `ValueError`:
```python
with pytest.raises(ValueError, match=r"Cannot remove .* do not find it in"):
c - [absent]
```
This also keeps the behavior aligned with the string-key path.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+250
to
+251
| if not isinstance(conditions, (list, tuple)): | ||
| conditions = [conditions] |
There was a problem hiding this comment.
suggestion: Normalising non-sequence conditions to a one-element list changes behaviour for other iterable types (e.g. generators); consider a more precise type check.
The current if not isinstance(conditions, (list, tuple)) branch fixes the scalar case but also wraps generators and other non-list/tuple sequences, changing them from multiple conditions into a single one.
To keep behaviour consistent for genuine sequences, consider checking for “is a scalar/non-sequence” rather than “is not list/tuple”. For example, you could detect collections.abc.Sequence (excluding str/bytes/np.ndarray) or explicitly handle np.bool_ and scalar arrays, so real sequences still flow through unchanged while scalars avoid the len() error.
Suggested implementation:
branches = [make_callable(branch) for branch in branches]
# A single condition may be passed as a bare scalar bool/array (the
# docstring types ``conditions`` as ``bool, sequence of bool``). Normalise
# true scalars into a one-element list so they flow through the conversion
# below; otherwise ``brainstate.transform.ifelse`` would call ``len()`` on
# the scalar and raise ``TypeError: object ... has no len()``. Existing
# iterable conditions (lists/tuples/generators/etc.) are left unchanged so
# they still represent multiple conditions.
is_numpy_scalar_bool = False
try:
import numpy as np # use existing import if already present
if isinstance(conditions, np.bool_) or (
isinstance(conditions, np.ndarray) and conditions.ndim == 0 and conditions.dtype == bool
):
is_numpy_scalar_bool = True
except Exception:
# NumPy not available or not used; fall back to pure-Python scalars only
pass
if isinstance(conditions, bool) or is_numpy_scalar_bool:
conditions = [conditions]
# Convert if-elif-else chain to mutually exclusive conditions
if isinstance(conditions, (list, tuple)) and len(conditions) > 0:
conditions = list(conditions)- If this file does not already import NumPy as
np, you may want to move theimport numpy as npto the module top-level instead of keeping the local import inside the function, to match existing style/import conventions. - If other scalar condition types (e.g.
np.int_or numeric scalars) should also be normalised, extend theis_numpy_scalar_boollogic and/or theisinstance(conditions, bool)check accordingly (e.g. usingnumbers.Integral), but the current change focuses on boolean scalars to match the documented type.
Comment on lines
+154
to
+160
| def test_sub_with_list_missing_value_raises(): | ||
| # P4-M3: removing a *value* object that is not present must raise the same | ||
| # descriptive ValueError as the string-key path, not a bare KeyError(id). | ||
| present = object() | ||
| absent = object() | ||
| c = Collector({'a': present}) | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
suggestion (testing): Strengthen the regression by checking the ValueError message content for missing values.
Since this already covers the missing-value path for Collector.__sub__, you can make the regression stronger by also asserting on the error message to distinguish it from a generic ValueError:
with pytest.raises(ValueError, match=r"Cannot remove .* do not find it in"):
c - [absent]This also keeps the behavior aligned with the string-key path.
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.
Fresh review of
brainpy/math/object_transform. Four Medium correctness/robustness fixes:bm.condnow accepts constant (non-callable) branches as documented.bm.ifelsenow accepts a scalar bool/array condition as documented.Collector.__sub__raises the descriptiveValueError(not a bareKeyError).VariableView.valuesetter mirrors the hardenedVariablesetter (unwrapsState, canonicalizes numpy dtype, handles plain inputs).Regression tests added (231 passed in-scope). Findings:
docs/issues-found-20260619-math-object-transform.md.Summary by Sourcery
Improve robustness and documented-behavior compliance of math object-transform utilities and add regression coverage for discovered edge cases.
Bug Fixes:
Documentation:
Tests: