Refactor GID handling and synapse seeding to align with Neurodamus#73
Refactor GID handling and synapse seeding to align with Neurodamus#73
Conversation
|
|
||
| If projections is None, all the synapses are extracted. | ||
| """ | ||
| """Extract the synapses.""" |
There was a problem hiding this comment.
Is the behavior of this method changed? IF not, why change the description?
There was a problem hiding this comment.
Yes we changed the behavior so that if projections is None, we don't add any.
| self.source_popid, self.target_popid = popids | ||
|
|
||
| self.pre_local_id = int(self.syn_description[SynapseProperty.PRE_GID]) | ||
| self.pre_gid = int(self.syn_description[SynapseProperty.PRE_GID]) |
There was a problem hiding this comment.
this line can be removed I believe if we are using pre_local_id
There was a problem hiding this comment.
Actually I think it is better to keep pre_gid and remove pre_local_id since pre_gid is used everywhere.
bluecellulab/circuit_simulation.py
Outdated
| if ( | ||
| user_pre_spike_trains is not None | ||
| and pre_gid in user_pre_spike_trains | ||
| user_pre_spike_trains is not None |
There was a problem hiding this comment.
this is overindented, no?
| single integer GID across all ranks. In SONATA circuits, node ids are only | ||
| unique *within* a population, so we combine (population_name, node_id) into a | ||
| single integer: | ||
| if self.gids is None: |
There was a problem hiding this comment.
Could you add dosctrings to your new functions please?
| else: | ||
| prev_count = int(max_raw[prev]) + 1 | ||
| end_prev = pop_offset[prev] + prev_count | ||
| pop_offset[p] = ((end_prev + 999) // 1000) * 1000 |
There was a problem hiding this comment.
It is the value that Neurodamus use to group each population to a 1000 sized block to keep gids non-overlapping.
bluecellulab/circuit/gid_resolver.py
Outdated
| def global_gid(self, pop: str, local_id: int) -> int: | ||
| return int(self.pop_offset[pop]) + int(local_id) + 1 # 1-based indexing to mirror Neurodamus synapse seeding implementation |
There was a problem hiding this comment.
I understand the intention of +1 here to match the Neurodamus synapse seeding. However, the name global_gid makes me confused that the gid is now 1-based. Neurodamus is using 0-based gids, the +1 is only for synapse seeding.
There was a problem hiding this comment.
Ah good point, I will move the +1 out of global_gid and apply it only in the seeding.
| rng_settings = RNGSettings.get_instance() | ||
| if rng_settings.mode == "Random123": | ||
| self.randseed1 = self.post_cell_id.id + 250 | ||
| self.randseed1 = self.post_gid + 250 |
There was a problem hiding this comment.
Continuing my previous comment about global_gid. If it is only used here, can we use instead self.randseed1 = self.post_gid (0-based) + 251 ?
And what is the different between self.post_cell_id.id and self.post_gid ?
There was a problem hiding this comment.
Maybe we don't need the new self.post_gid , but derive from self.post_cell_id.id ?
There was a problem hiding this comment.
I have added the +1 here, thank you!
post_cell_id.id is the node id from sonata (local to a population), while post_gid is the globally unique id following the Neurodamus convention.
There was a problem hiding this comment.
Then could you just pass the population offset instead of post_gid into this function?
There was a problem hiding this comment.
I would prefer to keep the gid computation higher up where the namespace is available and just pass the final value down. We don’t really have the population offset here and the synapse would still need to rebuild the gid. I also refactored a bit to avoid passing post_gid through a long chain and keep it on the cell.
3 participants
Refactored GID handling and synapse seeding to make simulations deterministic and consistent with Neurodamus. Global GIDs are now computed from SONATA node populations (1-based with 1000 block offsets). This introduces a breaking change: post_gid is now required when creating synapses.