fix(analysis): fixed-point classification, nullcline selection, GD batches, plot kwargs#849
Merged
Merged
Conversation
…tch count, plot kwargs
- 2D star vs degenerate-node stability classification was inverted and the STAR
branch was unreachable dead code; correctly distinguish star (scalar*I) from
defective node (Critical)
- phase-plane select_candidates='nullclines' tested the fy condition twice,
silently dropping all fx-nullcline candidate points; test fx OR fy (High)
- slow_points num_opt_loops = int(num_opt/num_batch) was 0 when num_opt<num_batch
-> empty losses -> concatenate crash; use max(1, ceil(...)) (Medium)
- streamplot used plot_style.get('linewidth') (not pop), passing linewidth twice
-> TypeError; use pop (Medium)
- set_markersize wrote to a typo local, leaving the module global stale (Medium)
Findings recorded in docs/issues-found-20260619-analysis.md
Reviewer's GuideFixes several correctness and robustness issues in the analysis package: corrects 2D star vs degenerate node classification, ensures nullcline-based fixed-point selection uses both nullclines, makes GD-based slow point search always run at least one batch, fixes a streamplot linewidth kwarg collision, and corrects plot marker size global state handling while adding regression tests and documentation of the audit findings. Sequence diagram for streamplot linewidth handling in plot_vector_fieldsequenceDiagram
actor User
participant PhasePlane2D
participant pyplot_streamplot
User->>PhasePlane2D: plot_vector_field(plot_method='streamplot', plot_style)
PhasePlane2D->>PhasePlane2D: plot_style.pop(linewidth, None)
PhasePlane2D->>PhasePlane2D: [compute linewidth if None]
PhasePlane2D->>pyplot_streamplot: streamplot(..., linewidth=linewidth, **plot_style)
Flow diagram for 2D star vs degenerate node stability classificationflowchart TD
A[Start stability_analysis 2D] --> B[Compute discriminant e]
B --> C{e == 0?}
C -- No --> Z[Use existing non-repeated classification]
C -- Yes --> D[Extract a,b,c,d from Jacobian]
D --> E{p < 0?}
E -- Yes --> F[Stable branch]
E -- No --> G[Unstable branch]
F --> H{b == 0 and c == 0 and a == d?}
G --> H
H -- Yes --> I[Return STABLE_STAR_2D or UNSTABLE_STAR_2D]
H -- No --> J[Return STABLE_DEGENERATE_2D or UNSTABLE_DEGENERATE_2D]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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, and left some high level feedback:
- In
stability_analysis, the scalar-multiple-of-identity check for distinguishing STAR vs DEGENERATE nodes is duplicated between the stable and unstable branches; consider extracting this into a small helper to reduce repetition and keep the classification logic in one place. - The new
num_opt_loops = max(1, int(np.ceil(num_opt / num_batch)))inSlowPointFinderwill run at least one batch even whennum_opt <= 0and can overshoot the requestednum_opt; it may be clearer to explicitly handlenum_opt <= 0(e.g., early-return) and to bound the total number of optimization steps tonum_optinside the loop.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `stability_analysis`, the scalar-multiple-of-identity check for distinguishing STAR vs DEGENERATE nodes is duplicated between the stable and unstable branches; consider extracting this into a small helper to reduce repetition and keep the classification logic in one place.
- The new `num_opt_loops = max(1, int(np.ceil(num_opt / num_batch)))` in `SlowPointFinder` will run at least one batch even when `num_opt <= 0` and can overshoot the requested `num_opt`; it may be clearer to explicitly handle `num_opt <= 0` (e.g., early-return) and to bound the total number of optimization steps to `num_opt` inside the loop.
## Individual Comments
### Comment 1
<location path="brainpy/analysis/lowdim/lowdim_phase_plane.py" line_range="226-235" />
<code_context>
+ # Pop (not get) so a user-supplied ``linewidth`` is consumed and
+ # only passed once -- otherwise it is forwarded both explicitly
+ # and via ``**plot_style`` -> TypeError (multiple values).
+ linewidth = plot_style.pop('linewidth', None)
if linewidth is None:
if (not np.isnan(dx).any()) and (not np.isnan(dy).any()):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `pop` here mutates `plot_style` and assumes it is a mutable mapping, which may surprise callers or break with non-dict mappings.
This changes behavior vs the previous `get`: it now mutates `plot_style` and requires it to implement `pop`. That can silently alter a dict that callers reuse after `plot_vector_field`, and can fail for generic `Mapping` instances. You can avoid both issues by copying before mutation (e.g. `plot_style = dict(plot_style)`) and popping from the copy used for `streamplot`, preserving the multiple-values fix without changing the caller’s object or type contract.
```suggestion
elif plot_method == 'streamplot':
if plot_style is None:
# Start from default style when no user style is provided.
plot_style = dict(arrowsize=1.2, density=1, color='thistle')
else:
# Work on a local copy to avoid mutating the caller's mapping
# (which may be a non-dict Mapping or reused elsewhere).
plot_style = dict(plot_style)
# Pop (not get) so a user-supplied ``linewidth`` is consumed and
# only passed once -- otherwise it is forwarded both explicitly
# and via ``**plot_style`` -> TypeError (multiple values).
linewidth = plot_style.pop('linewidth', None)
if linewidth is None:
if (not np.isnan(dx).any()) and (not np.isnan(dy).any()):
min_width, max_width = 0.5, 5.5
candidates = jnp.vstack(candidates)
```
</issue_to_address>
### Comment 2
<location path="brainpy/analysis/plotstyle_test.py" line_range="40-47" />
<code_context>
+ plotstyle.set_markersize(1.5)
+
+
+def test_set_plot_schema_validations():
+ with pytest.raises(TypeError):
+ plotstyle.set_plot_schema(123)
+ with pytest.raises(KeyError):
+ plotstyle.set_plot_schema('not-a-real-fixed-point-type')
+ # valid update
+ plotstyle.set_plot_schema(plotstyle.SADDLE_NODE, color='black')
+ assert plotstyle.plot_schema[plotstyle.SADDLE_NODE]['color'] == 'black'
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid leaking global `plot_schema` modifications across tests by restoring the original entry.
This test leaves `plotstyle.plot_schema[plotstyle.SADDLE_NODE]['color']` set to `'black'`, which can affect later tests that expect the default. Consider capturing the original entry at the start of the test and restoring it in a `try/finally` block (as done in `test_set_markersize_updates_global`) to keep tests isolated and order-independent.
</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
226
to
235
| elif plot_method == 'streamplot': | ||
| if plot_style is None: | ||
| plot_style = dict(arrowsize=1.2, density=1, color='thistle') | ||
| linewidth = plot_style.get('linewidth', None) | ||
| # Pop (not get) so a user-supplied ``linewidth`` is consumed and | ||
| # only passed once -- otherwise it is forwarded both explicitly | ||
| # and via ``**plot_style`` -> TypeError (multiple values). | ||
| linewidth = plot_style.pop('linewidth', None) | ||
| if linewidth is None: | ||
| if (not np.isnan(dx).any()) and (not np.isnan(dy).any()): | ||
| min_width, max_width = 0.5, 5.5 |
There was a problem hiding this comment.
suggestion (bug_risk): Using pop here mutates plot_style and assumes it is a mutable mapping, which may surprise callers or break with non-dict mappings.
This changes behavior vs the previous get: it now mutates plot_style and requires it to implement pop. That can silently alter a dict that callers reuse after plot_vector_field, and can fail for generic Mapping instances. You can avoid both issues by copying before mutation (e.g. plot_style = dict(plot_style)) and popping from the copy used for streamplot, preserving the multiple-values fix without changing the caller’s object or type contract.
Suggested change
| elif plot_method == 'streamplot': | |
| if plot_style is None: | |
| plot_style = dict(arrowsize=1.2, density=1, color='thistle') | |
| linewidth = plot_style.get('linewidth', None) | |
| # Pop (not get) so a user-supplied ``linewidth`` is consumed and | |
| # only passed once -- otherwise it is forwarded both explicitly | |
| # and via ``**plot_style`` -> TypeError (multiple values). | |
| linewidth = plot_style.pop('linewidth', None) | |
| if linewidth is None: | |
| if (not np.isnan(dx).any()) and (not np.isnan(dy).any()): | |
| min_width, max_width = 0.5, 5.5 | |
| elif plot_method == 'streamplot': | |
| if plot_style is None: | |
| # Start from default style when no user style is provided. | |
| plot_style = dict(arrowsize=1.2, density=1, color='thistle') | |
| else: | |
| # Work on a local copy to avoid mutating the caller's mapping | |
| # (which may be a non-dict Mapping or reused elsewhere). | |
| plot_style = dict(plot_style) | |
| # Pop (not get) so a user-supplied ``linewidth`` is consumed and | |
| # only passed once -- otherwise it is forwarded both explicitly | |
| # and via ``**plot_style`` -> TypeError (multiple values). | |
| linewidth = plot_style.pop('linewidth', None) | |
| if linewidth is None: | |
| if (not np.isnan(dx).any()) and (not np.isnan(dy).any()): | |
| min_width, max_width = 0.5, 5.5 | |
| candidates = jnp.vstack(candidates) |
Comment on lines
+40
to
+47
| def test_set_plot_schema_validations(): | ||
| with pytest.raises(TypeError): | ||
| plotstyle.set_plot_schema(123) | ||
| with pytest.raises(KeyError): | ||
| plotstyle.set_plot_schema('not-a-real-fixed-point-type') | ||
| # valid update | ||
| plotstyle.set_plot_schema(plotstyle.SADDLE_NODE, color='black') | ||
| assert plotstyle.plot_schema[plotstyle.SADDLE_NODE]['color'] == 'black' |
There was a problem hiding this comment.
suggestion (testing): Avoid leaking global plot_schema modifications across tests by restoring the original entry.
This test leaves plotstyle.plot_schema[plotstyle.SADDLE_NODE]['color'] set to 'black', which can affect later tests that expect the default. Consider capturing the original entry at the start of the test and restoring it in a try/finally block (as done in test_set_markersize_updates_global) to keep tests isolated and order-independent.
github-actions
Bot
added
documentation
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/analysis.±λImatrices now correctly classified as STAR.select_candidates='nullclines'tested thefycondition twice, silently dropping all fx-nullcline candidates; nowfx OR fy.slow_pointsnum_opt_loopswas 0 whennum_opt<num_batch-> empty losses ->concatenatecrash; nowmax(1, ceil(...)).linewidthtwice (.getnot.pop) ->TypeError.plotstyle.set_markersizewrote a typo local, leaving the module global stale.In-scope: 192 passed. Findings:
docs/issues-found-20260619-analysis.md.Summary by Sourcery
Fix multiple correctness and robustness issues in the analysis package, primarily around 2D stability classification, phase-plane fixed point selection, and high-dimensional slow point optimization, and add targeted regression tests and audit documentation.
Bug Fixes:
Enhancements:
Documentation:
Tests: