Refactor morphology and integrators into morph/quad subpackages #63
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
…ture and improve clarity
Contributor
Reviewer's GuideRefactors the project structure by moving morphology code into a new braincell.morph package and all integrators/solvers into a new braincell.quad package, updates imports and public API to match, refreshes documentation and changelogs for versions 0.0.5–0.0.7, bumps core BrainPy ecosystem dependencies, and standardizes copyright/branding to BrainX Ecosystem. Class diagram for refactored integrator and morphology core typesclassDiagram
class HHTypedNeuron {
}
class IonChannel {
}
class Channel {
}
class Ion {
}
class SingleCompartment {
}
class MultiCompartment {
}
class Morphology {
+from_swc(filename)
+from_asc(filename)
+visualize()
}
class Segment {
}
class Section {
}
class CylinderSection {
}
class PointSection {
}
class BranchingTree {
}
class DiffEqModule {
}
class DiffEqState {
}
class IndependentIntegration {
}
class quad_integrators {
+euler_step(target, args)
+rk4_step(target, args)
+exp_euler_step(target, args)
+ind_exp_euler_step(target, args, excluded_paths)
+backward_euler_step(target, args)
+implicit_euler_step(target, args)
+crank_nicolson_step(target, args)
+staggered_step(target, args)
}
class VoltageSolver {
}
class SodiumChannel {
}
class PotassiumChannel {
}
class CalciumChannel {
}
class HyperpolarizationActivatedChannel {
}
%% Inheritance and implementation
SingleCompartment --|> HHTypedNeuron
MultiCompartment --|> HHTypedNeuron
IonChannel --|> Channel
SodiumChannel --|> Channel
PotassiumChannel --|> Channel
CalciumChannel --|> Channel
HyperpolarizationActivatedChannel --|> Channel
%% Morphology composition
Morphology o-- Section
Section o-- Segment
CylinderSection --|> Section
PointSection --|> Section
Morphology --> BranchingTree
%% Integration protocol relations
HHTypedNeuron ..> DiffEqModule
HHTypedNeuron ..> DiffEqState
HHTypedNeuron ..> IndependentIntegration
quad_integrators ..> DiffEqModule
quad_integrators ..> DiffEqState
quad_integrators ..> IndependentIntegration
VoltageSolver ..> DiffEqState
MultiCompartment ..> VoltageSolver
Channel ..> DiffEqState
SodiumChannel ..> IndependentIntegration
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Member
Author
|
@sourcery-ai title |
sourcery-ai
bot
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
quad/_integrator_exp_euler.pythe@set_module_as('braincell')decorator is still used but the import ofset_module_aswas removed, which will raise a NameError at import time—either reintroduce the import (frombraincell._misc) or drop the decorator. - The new
braincell.morphandbraincell.quadpackages have empty__init__.pyfiles whilebraincell/__init__.pynow doesfrom .morph import *andfrom .quad import *; if you intend to preserve the previous public API, you should re-export the key symbols (e.g.Morphology,Segment, integrator functions, protocols) from those package__init__modules. - There are now several places importing from the new sub-packages (e.g.
from .quad import DiffEqState, IndependentIntegration), but any remaining external code or docs that reference the old private modules (likebraincell._integrator_protocol,braincell._morphology) will break; consider adding thin shims or explicit deprecation paths if backward compatibility is desired.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `quad/_integrator_exp_euler.py` the `@set_module_as('braincell')` decorator is still used but the import of `set_module_as` was removed, which will raise a NameError at import time—either reintroduce the import (from `braincell._misc`) or drop the decorator.
- The new `braincell.morph` and `braincell.quad` packages have empty `__init__.py` files while `braincell/__init__.py` now does `from .morph import *` and `from .quad import *`; if you intend to preserve the previous public API, you should re-export the key symbols (e.g. `Morphology`, `Segment`, integrator functions, protocols) from those package `__init__` modules.
- There are now several places importing from the new sub-packages (e.g. `from .quad import DiffEqState, IndependentIntegration`), but any remaining external code or docs that reference the old private modules (like `braincell._integrator_protocol`, `braincell._morphology`) will break; consider adding thin shims or explicit deprecation paths if backward compatibility is desired.
## Individual Comments
### Comment 1
<location> `braincell/quad/_integrator_exp_euler.py:179` </location>
<code_context>
@set_module_as('braincell')
-def ind_exp_euler_step(target: DiffEqModule, *args, excluded_paths=()):
+def ind_exp_euler_step(target: DiffEqModule, *args, excluded_paths=()):
</code_context>
<issue_to_address>
**issue (bug_risk):** set_module_as is still used here but its import was removed, which will cause a NameError at runtime.
Please reintroduce the missing import for this decorator, e.g. `from braincell._misc import set_module_as` or an equivalent relative import that matches the new package layout, so the module can be imported without error.
</issue_to_address>
### Comment 2
<location> `docs/apis/braincell.rst:9-18` </location>
<code_context>
- structure.rst
- integration.rst
- morphology.rst
+Base Class for Cell Modeling
+----------------------------
+
+.. autosummary::
+ :toctree: generated/
+ :nosignatures:
+ :template: classtemplate.rst
+
+ HHTypedNeuron
+ SingleCompartment
+ MultiCompartment
+
+
+
+Base Class for Ion Channels
+---------------------------
+
</code_context>
<issue_to_address>
**suggestion (typo):** Consider using plural "Base Classes" in these section headings to match the multiple classes listed.
Since each section lists multiple classes (e.g., `HHTypedNeuron`, `SingleCompartment`, `MultiCompartment`), updating the headings to "Base Classes for Cell Modeling" and "Base Classes for Ion Channels" would be more natural and grammatically accurate.
Suggested implementation:
```
Base Classes for Cell Modeling
------------------------------
```
There is likely a similar section later in the same file that currently reads:
`Base Class for Ion Channels`
with an underline of dashes.
That heading should be updated analogously to:
`Base Classes for Ion Channels`
and the underline length should be adjusted (or simply extended) to at least match the new title length.
</issue_to_address>
### Comment 3
<location> `docs/apis/changelog.md:52` </location>
<code_context>
+ - `SC03_COBA_HH_2007_braincell.py`
+ - `SC05_thalamus_single_compartment_neurons.py`
+ - `SC06_unified_thalamus_model.py`
+ - `SC07_Straital_beta_oscillation_2011.py`
+ - `MC11_simple_dendrite_model.py`
+ - `MC13_golgi_model/` simulations
</code_context>
<issue_to_address>
**question (typo):** Check spelling of "Straital" in the example script name.
"Straital" in `SC07_Straital_beta_oscillation_2011.py` is likely a misspelling of the anatomical term "Striatal". If so, please update the filename and this reference accordingly, or add a brief note if the current name is intentional.
```suggestion
- `SC07_Striatal_beta_oscillation_2011.py`
```
</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
+9
to
+18
| Base Class for Cell Modeling | ||
| ---------------------------- | ||
|
|
||
| .. autosummary:: | ||
| :toctree: generated/ | ||
| :nosignatures: | ||
| :template: classtemplate.rst | ||
|
|
||
| HHTypedNeuron | ||
| SingleCompartment |
Contributor
There was a problem hiding this comment.
suggestion (typo): Consider using plural "Base Classes" in these section headings to match the multiple classes listed.
Since each section lists multiple classes (e.g., HHTypedNeuron, SingleCompartment, MultiCompartment), updating the headings to "Base Classes for Cell Modeling" and "Base Classes for Ion Channels" would be more natural and grammatically accurate.
Suggested implementation:
Base Classes for Cell Modeling
------------------------------
There is likely a similar section later in the same file that currently reads:
Base Class for Ion Channels
with an underline of dashes.
That heading should be updated analogously to:
Base Classes for Ion Channels
and the underline length should be adjusted (or simply extended) to at least match the new title length.
| - `SC03_COBA_HH_2007_braincell.py` | ||
| - `SC05_thalamus_single_compartment_neurons.py` | ||
| - `SC06_unified_thalamus_model.py` | ||
| - `SC07_Straital_beta_oscillation_2011.py` |
Contributor
There was a problem hiding this comment.
question (typo): Check spelling of "Straital" in the example script name.
"Straital" in SC07_Straital_beta_oscillation_2011.py is likely a misspelling of the anatomical term "Striatal". If so, please update the filename and this reference accordingly, or add a brief note if the current name is intentional.
Suggested change
| - `SC07_Straital_beta_oscillation_2011.py` | |
| - `SC07_Striatal_beta_oscillation_2011.py` |
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 the package structure by extracting morphology and integrator functionality into dedicated sub-packages, update dependencies for the latest BrainPy ecosystem, and refresh documentation and examples to match the new APIs and branding.
Enhancements:
Build:
CI:
Documentation:
Tests:
Chores: