Standardize Units, Refactor Current Handling, and Enhance Examples#44
Merged
chaoming0625 merged 1 commit intomainfrom Jul 25, 2025
Merged
Standardize Units, Refactor Current Handling, and Enhance Examples#44chaoming0625 merged 1 commit intomainfrom
chaoming0625 merged 1 commit intomainfrom
Conversation
…e hints and improve documentation
Contributor
Reviewer's GuideThis PR refactors the external current registration API and ion channel error handling, and brings unit consistency and formatting improvements to the example scripts. Class diagram for refactored external current registration and error handlingclassDiagram
class Ion {
_external_currents: dict
+register_external_current(key: Hashable, fun: Callable)
}
class Channel {
name
root_type
}
class IonManager {
+add(**elements)
_get_ion_fun(ion, elem)
}
IonManager --> Channel : add
Channel --> Ion : register_external_current
Ion <.. IonManager : uses
note for Ion "register_external_current now takes Hashable key and improved error message"
note for IonManager "add() now registers external current with id(elem) as key"
Class diagram for improved error handling in compute_derivativeclassDiagram
class SingleCompartment {
+compute_derivative(I_ext=0.*u.nA/u.cm**2)
V
C
+sum_current_inputs(I_ext, V)
+nodes(type, allowed_hierarchy)
}
class IonChannel {
+current(V)
+compute_derivative(V)
}
SingleCompartment --> IonChannel : nodes()
note for SingleCompartment "compute_derivative now wraps IonChannel.current in try/except and raises ValueError with context"
Class diagram for ThalamusNeuron compute_derivative overrideclassDiagram
class ThalamusNeuron {
+compute_derivative(I_ext=0.*u.nA)
area
V
C
+sum_current_inputs(I_ext, V)
+nodes(type, allowed_hierarchy)
}
ThalamusNeuron --|> SingleCompartment
class SingleCompartment {
+compute_derivative(I_ext=0.*u.nA/u.cm**2)
}
ThalamusNeuron --> IonChannel : nodes()
note for ThalamusNeuron "compute_derivative now multiplies I_ext by area and matches new error handling"
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}
Contributor
There was a problem hiding this comment.
Hey @chaoming0625 - I've reviewed your changes - here's some feedback:
- The repeated synapse setup patterns (align_pre, AMPA/GABAa parameters, comm, out) could be refactored into a helper or factory to reduce boilerplate and improve readability.
- Avoid using Python’s id(elem) for external current registration keys—consider a stable, descriptive identifier (e.g. elem.name) to make debugging and introspection easier.
- When wrapping exceptions in compute_derivative, use “raise ValueError(…) from e” so the original traceback is preserved for easier debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated synapse setup patterns (align_pre, AMPA/GABAa parameters, comm, out) could be refactored into a helper or factory to reduce boilerplate and improve readability.
- Avoid using Python’s id(elem) for external current registration keys—consider a stable, descriptive identifier (e.g. elem.name) to make debugging and introspection easier.
- When wrapping exceptions in compute_derivative, use “raise ValueError(…) from e” so the original traceback is preserved for easier debugging.
## Individual Comments
### Comment 1
<location> `examples/SC06_unified_thalamus_model.py:392` </location>
<code_context>
elements = np.where(spikes > 0.)
index = elements[1]
time = times[elements[0]]
ax.plot(time, index, marker + color, markersize=markersize)
if xlabel:
- ax.xlabel(xlabel)
+ ax.set_xlabel(xlabel)
if ylabel:
- ax.ylabel(ylabel)
+ ax.set_ylabel(ylabel)
if xlim:
- ax.xlim(xlim)
+ ax.set_xlim(xlim)
</code_context>
<issue_to_address>
Fixed incorrect method calls for axis labeling and limits.
Using the correct set_* methods prevents attribute errors when labeling axes and setting limits in matplotlib.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
elements = np.where(spikes > 0.)
index = elements[1]
time = times[elements[0]]
=======
elements = np.where(spikes > 0.)
index = elements[1]
time = times[elements[0]]
ax.plot(time, index, marker + color, markersize=markersize)
if xlabel:
ax.set_xlabel(xlabel)
if ylabel:
ax.set_ylabel(ylabel)
if xlim:
ax.set_xlim(xlim)
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `braincell/_single_compartment.py:229` </location>
<code_context>
for key, ch in self.nodes(IonChannel, allowed_hierarchy=(1, 1)).items():
- I_ext = I_ext + ch.current(self.V.value)
-
+ try:
+ I_ext = I_ext + ch.current(self.V.value)
+ except Exception as e:
+ raise ValueError(
+ f"Error in computing current for ion channel '{key}': \n"
+ f"{ch}\n"
+ f"Error: {e}"
+ )
self.V.derivative = I_ext / self.C
-
</code_context>
<issue_to_address>
Added error handling for ion channel current computation.
Consider handling more specific exceptions instead of catching all Exceptions to prevent masking unrelated errors.
</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
389
to
391
| elements = np.where(spikes > 0.) | ||
| index = elements[1] | ||
| time = times[elements[0]] |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Fixed incorrect method calls for axis labeling and limits.
Using the correct set_* methods prevents attribute errors when labeling axes and setting limits in matplotlib.
Suggested change
| elements = np.where(spikes > 0.) | |
| index = elements[1] | |
| time = times[elements[0]] | |
| elements = np.where(spikes > 0.) | |
| index = elements[1] | |
| time = times[elements[0]] | |
| ax.plot(time, index, marker + color, markersize=markersize) | |
| if xlabel: | |
| ax.set_xlabel(xlabel) | |
| if ylabel: | |
| ax.set_ylabel(ylabel) | |
| if xlim: | |
| ax.set_xlim(xlim) |
Comment on lines
+229
to
+236
| try: | ||
| I_ext = I_ext + ch.current(self.V.value) | ||
| except Exception as e: | ||
| raise ValueError( | ||
| f"Error in computing current for ion channel '{key}': \n" | ||
| f"{ch}\n" | ||
| f"Error: {e}" | ||
| ) |
Contributor
There was a problem hiding this comment.
suggestion: Added error handling for ion channel current computation.
Consider handling more specific exceptions instead of catching all Exceptions to prevent masking unrelated errors.
| f"Error in computing current for ion channel '{key}': \n" | ||
| f"{ch}\n" | ||
| f"Error: {e}" | ||
| ) |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Suggested change
| ) | |
| ) from e |
Collaborator
Author
|
@sourcery-ai title |
sourcery-ai
Bot
changed the title
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.
Summary by Sourcery
Improve unit consistency and debugging in neuron and network models, refine external current registration, and enhance example scripts with better plotting and progress reporting.
Bug Fixes:
Enhancements: