Add SONATA point-process cell support#51
Conversation
The logic for loading from sonata files is to swap out the list of parameters if the BBP TYPE is not present Likewise, when instantiating a synapse object, if no TYPE field is present, assume it is for allen and create a Exp2Syn synapse
…e instantiated. The reporting adds try/except since these don't have a soma
… target point neurons and deliver replay spikes to the proper netcon
Removed call to finalize for point process type cells since this is not needed
Updated allen tests to collection cell_info and get ExpSyn data as consequence
…re netcon code Remove more unused code sections; can be added when use case example is available
Added additional test for invalid Mechanism given to HocPointProcessCell
darshanmandge
left a comment
darshanmandge
left a comment
There was a problem hiding this comment.
Thank you, @jamesgking, for adding this to bluecellulab! I have left some comments.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class BasePointProcessCell(Cell): |
There was a problem hiding this comment.
This inherits from Cell but never calls super().__init__(), so none of Cell's attributes (self.recordings, self.soma, self.cell, self.record_dt, etc.) are initialized. This means inherited methods like get_time(), get_recording(), etc., will crash with AttributeError. The bare except Exception: continue blocks added in utils.py may catch the errors though. Maybe initialising the minimum required attributes so inherited methods degrade gracefully instead of relying on catch-all exception handlers.
There was a problem hiding this comment.
Let's sync on this since originally this did not inherit, but this caused the linting to throw errors, I hear what you say about how inheriting should conform to the expected interface
There was a problem hiding this comment.
The linting issue can be because CircuitSimulation type-hints cells as Cell, so without inheritance the type checker complains. We can either:
- Keep the inheritance but add a minimal set of Cell-compatible attributes in
BasePointProcessCell.__init__ (recordings={}, soma=None, report_sites={}, etc.). It is a few lines change. - Extract a shared
BaseCellABC that both Cell andBasePointProcessCellimplement, and updateCircuitSimulationto reference that. This is cleaner but larger scope.
WDYT?
There was a problem hiding this comment.
I can move forward with adding the attributes to BasePointProcessCell for now. I can try to simply init all the same fields with None/empty and see if tests fail
| point_params = PointProcessConnParameters(syn_description[SynapseProperty.PRE_GID], syn_description[SynapseProperty.PRE_GID], | ||
| syn_description[SynapseProperty.AXONAL_DELAY]) | ||
|
|
||
| self.pointConn = PointProcessConnection([point_params]) |
There was a problem hiding this comment.
This is assigned but never stored in self.connections or used anywhere else. The connection object is created and then effectively discarded on the next call. Is this intentional?
There was a problem hiding this comment.
properly stored now. Note that additional fields are needed when accessed later. I would like to discuss possible better organization of what I wrote
There was a problem hiding this comment.
Thanks. The change is fine.
If you want, the attributes (syn_description, delay_weights, hsynapse, syn_id) could be moved to PointProcessConnection.__init__ params with None defaults. Right now they're set externally after construction, which means the class definition doesn't document its own full interface.
Additionally, PointProcessConnection.info_dict calls self.syn_description.to_dict() without a check, if the object is ever constructed without setting syn_description, it'll crash. A small fix:
@property
def info_dict(self):
synapse_dict: dict[str, Any] = {}
if self.syn_description is not None:
synapse_dict['syn_description'] = self.syn_description.to_dict()
synapse_dict['syn_description'] = {
str(k): v for k, v in synapse_dict['syn_description'].items()}
return synapse_dict
This is mainly about making the class crash-safe if used elsewhere in the future.
| point_params = PointProcessConnParameters(syn_description[SynapseProperty.PRE_GID], syn_description[SynapseProperty.PRE_GID], | ||
| syn_description[SynapseProperty.AXONAL_DELAY]) | ||
|
|
||
| self.pointConn = PointProcessConnection([point_params]) |
There was a problem hiding this comment.
How about the weight_factor attribute in PointProcessConnection ? Is it assigned somewhere from the connection_override config file ?
There was a problem hiding this comment.
Code refactored and now accessed proper
There was a problem hiding this comment.
syn_connection_parameters is still received but unused in add_replay_synapse. The PointProcessConnection is created without weight_factor, so it always defaults to 1.0. We can extract the weight from syn_connection_parameters.get("Weight", 1.0) and pass it through.
Refactor to conform better to existing BlueCellulab structures
4 participants
This PR adds support for SONATA point-process cells.
Point-process cells are instantiated from SONATA model_type = "point_process" and model_template, mapping directly to NEURON point mechanisms defined in HOC/MOD files.
What is included