Refactor GID handling and synapse seeding to align with Neurodamus#73
Refactor GID handling and synapse seeding to align with Neurodamus#73
Conversation
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?
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.
AurelienJaquier
left a comment
AurelienJaquier
left a comment
There was a problem hiding this comment.
looks good to me
| s = node_pop.size | ||
| out[str(pop_name)] = int(s() if callable(s) else s) |
There was a problem hiding this comment.
Is node_pop not always a libsonata NodePopulation ? Why do you need to detect if it is callable ?
There was a problem hiding this comment.
Right, NodePopulation.size is a cached property. I have droped the check
bluecellulab/circuit_simulation.py
Outdated
| def _build_gid_namespace(self) -> GidNamespace: | ||
| sizes = self.circuit_access.node_population_sizes() | ||
| pops_sorted = sorted(sizes.keys()) | ||
| max_raw = {p: max(0, int(n) - 1) for p, n in sizes.items()} |
There was a problem hiding this comment.
One more question, sorry.
max_raw here is either 0 or the population size?
But in Neurodamus we use the max gid of the loaded cells
https://github.com/openbraininstitute/neurodamus/blob/e5abd33ad2cc5a4450c2d5190e32afc11ee385d8/neurodamus/core/nodeset.py#L231
For virtual population, it is the whole population size
https://github.com/openbraininstitute/neurodamus/blob/e5abd33ad2cc5a4450c2d5190e32afc11ee385d8/neurodamus/node.py#L130
The offset would be different between BlueCellulab and Neurodamus if they don't load the full population.
There was a problem hiding this comment.
Great catch! I have aligned it now so we use the max gid from the loaded cells and the full population size for virtual populations. Let me know if anything still looks off, thanks!
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.