Update version to 2.7.1 and refactor Dynamics class: enhance input ha… #793
Conversation
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
…ndling and add new properties
Reviewer's GuideThis PR refactors and enriches the core Dynamics class in brainpy.state to provide a unified input-handling API, updates Neuron/Synapse and projection modules to adopt it, cleans up outdated docs, and bumps the package version to 2.7.1. Class diagram for refactored Dynamics, Neuron, and Synapse classesclassDiagram
class Dynamics {
+__init__(in_size: Size, name: Optional[str] = None)
+current_inputs: dict | None
+delta_inputs: dict | None
+add_current_input(key: str, inp: Callable|ArrayLike, label: Optional[str] = None)
+add_delta_input(key: str, inp: Callable|ArrayLike, label: Optional[str] = None)
+get_input(key: str)
+sum_current_inputs(init: Any, *args, label: Optional[str] = None, **kwargs)
+sum_delta_inputs(init: Any, *args, label: Optional[str] = None, **kwargs)
+align_pre(dyn: ParamDescriber[T] | T) -> T
}
class Neuron {
}
class Synapse {
}
Neuron --|> Dynamics
Synapse --|> Dynamics
Class diagram for updated projection module using DynamicsclassDiagram
class _AlignPost {
+__init__(syn: Dynamics, out: BindCondData)
+syn: Dynamics
+out: BindCondData
}
class Projection {
}
class align_pre_projection {
+__init__(*spike_generator, syn: Dynamics, comm: Callable, out: SynOut, post: Dynamics, stp: Dynamics = None)
}
_AlignPost --|> brainstate.nn.Module
align_pre_projection --|> Projection
_AlignPost o-- Dynamics
align_pre_projection o-- Dynamics
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 there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `brainpy/state/_projection.py:131-132` </location>
<code_context>
# synapse and output initialization
post.add_current_input(proj_name, out_cls, label=label)
- post._add_before_update(_post_repr, _AlignPost(syn_cls, out_cls))
- syn = post._get_before_update(_post_repr).syn
- out = post._get_before_update(_post_repr).out
+ post.add_before_update(_post_repr, _AlignPost(syn_cls, out_cls))
+ syn = post.get_before_update(_post_repr).syn
+ out = post.get_before_update(_post_repr).out
</code_context>
<issue_to_address>
**suggestion:** Consider documenting the expected interface for Dynamics extensions.
Documenting these required methods will help developers correctly implement extensions to the Dynamics class.
Suggested implementation:
```python
class Dynamics:
"""
Base class for synaptic dynamics extensions.
Extensions to Dynamics should implement the following methods:
- update(self, ...): Update the state of the synapse.
- reset(self, ...): Reset the synapse state.
- __call__(self, ...): Compute the synaptic output.
Additional methods may be required depending on the specific extension.
See the documentation for details.
"""
# Existing implementation...
class _AlignPost(brainstate.nn.Module):
def __init__(
self,
syn: Dynamics,
out: BindCondData
):
```
If the `Dynamics` class is defined in another file, you should add the docstring there instead.
If you have custom extension points or abstract methods, list them explicitly in the docstring.
</issue_to_address>
### Comment 2
<location> `brainpy/state/_base.py:139` </location>
<code_context>
self._current_inputs = dict()
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace `dict()` with `{}` ([`dict-literal`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dict-literal))
```suggestion
self._current_inputs = {}
```
<br/><details><summary>Explanation</summary>The most concise and Pythonic way to create a dictionary is to use the `{}`
notation.
This fits in with the way we create dictionaries with items, saving a bit of
mental energy that might be taken up with thinking about two different ways of
creating dicts.
```python
x = {"first": "thing"}
```
Doing things this way has the added advantage of being a nice little performance
improvement.
Here are the timings before and after the change:
```
$ python3 -m timeit "x = dict()"
5000000 loops, best of 5: 69.8 nsec per loop
```
```
$ python3 -m timeit "x = {}"
20000000 loops, best of 5: 29.4 nsec per loop
```
Similar reasoning and performance results hold for replacing `list()` with `[]`.
</details>
</issue_to_address>
### Comment 3
<location> `brainpy/state/_base.py:140-142` </location>
<code_context>
if key in self._current_inputs:
if id(self._current_inputs[key]) != id(inp):
raise ValueError(f'Key "{key}" has been defined and used in the current inputs of {self}.')
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if key in self._current_inputs and id(self._current_inputs[key]) != id(inp):
raise ValueError(f'Key "{key}" has been defined and used in the current inputs of {self}.')
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 4
<location> `brainpy/state/_base.py:190` </location>
<code_context>
self._delta_inputs = dict()
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace `dict()` with `{}` ([`dict-literal`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dict-literal))
```suggestion
self._delta_inputs = {}
```
<br/><details><summary>Explanation</summary>The most concise and Pythonic way to create a dictionary is to use the `{}`
notation.
This fits in with the way we create dictionaries with items, saving a bit of
mental energy that might be taken up with thinking about two different ways of
creating dicts.
```python
x = {"first": "thing"}
```
Doing things this way has the added advantage of being a nice little performance
improvement.
Here are the timings before and after the change:
```
$ python3 -m timeit "x = dict()"
5000000 loops, best of 5: 69.8 nsec per loop
```
```
$ python3 -m timeit "x = {}"
20000000 loops, best of 5: 29.4 nsec per loop
```
Similar reasoning and performance results hold for replacing `list()` with `[]`.
</details>
</issue_to_address>
### Comment 5
<location> `brainpy/state/_base.py:191-193` </location>
<code_context>
if key in self._delta_inputs:
if id(self._delta_inputs[key]) != id(inp):
raise ValueError(f'Key "{key}" has been defined and used.')
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if key in self._delta_inputs and id(self._delta_inputs[key]) != id(inp):
raise ValueError(f'Key "{key}" has been defined and used.')
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 6
<location> `brainpy/state/_base.py:39` </location>
<code_context>
def _input_label_repr(name: str, label: Optional[str] = None):
# unify the input label repr.
return name if label is None else (_input_label_start(label) + str(name))
</code_context>
<issue_to_address>
**suggestion (code-quality):** Remove unnecessary casts to int, str, float or bool ([`remove-unnecessary-cast`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-unnecessary-cast/))
```suggestion
return name if label is None else _input_label_start(label) + name
```
</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
-131
to
-132
| post._add_before_update(_post_repr, _AlignPost(syn_cls, out_cls)) | ||
| syn = post._get_before_update(_post_repr).syn |
There was a problem hiding this comment.
suggestion: Consider documenting the expected interface for Dynamics extensions.
Documenting these required methods will help developers correctly implement extensions to the Dynamics class.
Suggested implementation:
class Dynamics:
"""
Base class for synaptic dynamics extensions.
Extensions to Dynamics should implement the following methods:
- update(self, ...): Update the state of the synapse.
- reset(self, ...): Reset the synapse state.
- __call__(self, ...): Compute the synaptic output.
Additional methods may be required depending on the specific extension.
See the documentation for details.
"""
# Existing implementation...
class _AlignPost(brainstate.nn.Module):
def __init__(
self,
syn: Dynamics,
out: BindCondData
):If the Dynamics class is defined in another file, you should add the docstring there instead.
If you have custom extension points or abstract methods, list them explicitly in the docstring.
| """ | ||
| key = _input_label_repr(key, label) | ||
| if self._current_inputs is None: | ||
| self._current_inputs = dict() |
There was a problem hiding this comment.
suggestion (code-quality): Replace dict() with {} (dict-literal)
Suggested change
| self._current_inputs = dict() | |
| self._current_inputs = {} |
Explanation
The most concise and Pythonic way to create a dictionary is to use the{}notation.
This fits in with the way we create dictionaries with items, saving a bit of
mental energy that might be taken up with thinking about two different ways of
creating dicts.
x = {"first": "thing"}Doing things this way has the added advantage of being a nice little performance
improvement.
Here are the timings before and after the change:
$ python3 -m timeit "x = dict()"
5000000 loops, best of 5: 69.8 nsec per loop
$ python3 -m timeit "x = {}"
20000000 loops, best of 5: 29.4 nsec per loop
Similar reasoning and performance results hold for replacing list() with [].
Comment on lines
+140
to
+142
| if key in self._current_inputs: | ||
| if id(self._current_inputs[key]) != id(inp): | ||
| raise ValueError(f'Key "{key}" has been defined and used in the current inputs of {self}.') |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
Suggested change
| if key in self._current_inputs: | |
| if id(self._current_inputs[key]) != id(inp): | |
| raise ValueError(f'Key "{key}" has been defined and used in the current inputs of {self}.') | |
| if key in self._current_inputs and id(self._current_inputs[key]) != id(inp): | |
| raise ValueError(f'Key "{key}" has been defined and used in the current inputs of {self}.') | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
| """ | ||
| key = _input_label_repr(key, label) | ||
| if self._delta_inputs is None: | ||
| self._delta_inputs = dict() |
There was a problem hiding this comment.
suggestion (code-quality): Replace dict() with {} (dict-literal)
Suggested change
| self._delta_inputs = dict() | |
| self._delta_inputs = {} |
Explanation
The most concise and Pythonic way to create a dictionary is to use the{}notation.
This fits in with the way we create dictionaries with items, saving a bit of
mental energy that might be taken up with thinking about two different ways of
creating dicts.
x = {"first": "thing"}Doing things this way has the added advantage of being a nice little performance
improvement.
Here are the timings before and after the change:
$ python3 -m timeit "x = dict()"
5000000 loops, best of 5: 69.8 nsec per loop
$ python3 -m timeit "x = {}"
20000000 loops, best of 5: 29.4 nsec per loop
Similar reasoning and performance results hold for replacing list() with [].
Comment on lines
+191
to
+193
| if key in self._delta_inputs: | ||
| if id(self._delta_inputs[key]) != id(inp): | ||
| raise ValueError(f'Key "{key}" has been defined and used.') |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
Suggested change
| if key in self._delta_inputs: | |
| if id(self._delta_inputs[key]) != id(inp): | |
| raise ValueError(f'Key "{key}" has been defined and used.') | |
| if key in self._delta_inputs and id(self._delta_inputs[key]) != id(inp): | |
| raise ValueError(f'Key "{key}" has been defined and used.') | |
Explanation
Too much nesting can make code difficult to understand, and this is especiallytrue in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two if conditions can be combined using
and is an easy win.
|
|
||
| def _input_label_repr(name: str, label: Optional[str] = None): | ||
| # unify the input label repr. | ||
| return name if label is None else (_input_label_start(label) + str(name)) |
There was a problem hiding this comment.
suggestion (code-quality): Remove unnecessary casts to int, str, float or bool (remove-unnecessary-cast)
Suggested change
| return name if label is None else (_input_label_start(label) + str(name)) | |
| return name if label is None else _input_label_start(label) + name |
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 by Sourcery
Refactor core state Dynamics class to support flexible current and delta input management, update Neuron and Synapse to use the new Dynamics base, adjust projection alignment logic to the new API, remove outdated example notebooks, and bump package version to 2.7.1.
New Features:
Enhancements:
Documentation:
Chores: