fix: IntegratorRunner dict-monitor regression + stale post-audit test updates#854
Merged
Merged
Conversation
…ug-pinning tests Source fix ---------- `IntegratorRunner` dict monitors regressed: after `_format_dict_monitors` each value is a ``(var_name_or_Variable, index)`` tuple, but the resolution code tested ``isinstance(i, str)`` on that tuple (always False), so a string variable-name was passed through to the base runner, which then looked it up as an attribute of ``self.target`` and failed. Integrator state variables live in ``self.variables``, so resolve string names there before handing the base runner an already-resolved ``(Variable, index)`` pair. Test updates ------------ Several post-audit tests pinned now-fixed behaviour. Re-pointed them at the corrected contract: - nvar_coverage_test: order < 2 now raises ValueError (not AssertionError). - dnn_toolbox_fixes_test: Adan now constructs and updates cleanly. - boost_misc_test: offline ElasticNet / Logistic regression now fit & predict. - train_analysis_glue_fixes_test: logistic regression now fits without error.
Reviewer's GuideFixes an IntegratorRunner dict-monitor regression by correctly resolving string variable names to integrator state variables, and updates several tests that previously pinned buggy behavior so they now assert on the corrected, successful paths for ElasticNet/Logistic regression, Adan optimizer updates, and NVAR error typing. Sequence diagram for IntegratorRunner dict monitor resolution fixsequenceDiagram
actor User
participant IntegratorRunner
participant Variables
participant BaseRunner
User->>IntegratorRunner: __init__(target, monitors)
alt monitors is dict
IntegratorRunner->>IntegratorRunner: _format_dict_monitors(monitors)
loop each_monitor_entry
IntegratorRunner->>Variables: variables[var_name]
Variables-->>IntegratorRunner: Variable
IntegratorRunner->>IntegratorRunner: build (Variable, index) pair
end
end
IntegratorRunner->>BaseRunner: __init__(target, resolved_monitors)
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
IntegratorRunner.__init__, the dict-monitor normalization comprehension relies on tuple indexing (v[0], v[1]) and silently passes through non-tuple values; consider unpacking with pattern matching or explicit validation (e.g.,for k, (var, idx) in monitors.items()) to make the expected structure clearer and fail fast on malformed monitor specs. - The updated test docstrings embed audit dates and process context (e.g., "Fixed in audit 2026-06-19"), which can quickly go stale; consider trimming these to describe only the expected behavior and failure mode so the tests remain accurate without historical context.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `IntegratorRunner.__init__`, the dict-monitor normalization comprehension relies on tuple indexing (`v[0], v[1]`) and silently passes through non-tuple values; consider unpacking with pattern matching or explicit validation (e.g., `for k, (var, idx) in monitors.items()`) to make the expected structure clearer and fail fast on malformed monitor specs.
- The updated test docstrings embed audit dates and process context (e.g., "Fixed in audit 2026-06-19"), which can quickly go stale; consider trimming these to describe only the expected behavior and failure mode so the tests remain accurate without historical context.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.
Summary
Post-integration follow-up to the 16-part audit (PRs #838–#853). Fixes one
real regression surfaced by the full-suite run and re-points five stale
bug-pinning tests at the now-corrected behaviour.
Source fix
brainpy/integrators/runner.py—IntegratorRunnerdict monitorsregressed: after
_format_dict_monitorseach value is a(var_name_or_Variable, index)tuple, but resolution testedisinstance(i, str)on the whole tuple (always False), so a stringvariable-name leaked through to the base runner, which then looked it up
as an attribute of
self.targetand raised. Integrator state variableslive in
self.variables; resolve string names there and pass the baserunner an already-resolved
(Variable, index)pair.Test updates (behaviour now correct, tests re-pointed)
nvar_coverage_test—order < 2now raisesValueError(notAssertionError).dnn_toolbox_fixes_test— Adan now constructs and updates cleanly.boost_misc_test— offline ElasticNet / Logistic regression now fit & predict.train_analysis_glue_fixes_test— logistic regression now fits without error.Test plan
IntegratorRunnerdict monitors andDSRunnerdict monitors both run green.Summary by Sourcery
Fix IntegratorRunner dictionary monitor handling and update tests to reflect corrected optimizer and offline regression behavior.
Bug Fixes:
Tests: