[WIP] Automatic handling of relationships between objects #588
Conversation
|
Hello @apdavison! Thanks for updating the PR.
Comment last updated on October 23, 2018 at 07:55 Hours UTC |
|
Hi @apdavison , My first impression is that we should first implement in a generic class that do the bidirectional relationship for all object (at least the one to many) and then hinerits from that generic class. Then this SpikeTrainList would inherits this generic class. Naively, I would inherits this generic class from the python list but I don't known the consequence. Cheers, Samuel |
| } | ||
| return obj | ||
|
|
||
| def _spiketrains_from_array(self): |
There was a problem hiding this comment.
This function is quite expensive, as it generates all Spiketrain objects explicitly. As it is used in many of Spiketrainlist methods, this would cause quite a bit of overhead. Maybe it would be good to use it in only as few places as possible?
There was a problem hiding this comment.
It's only ever used once, because the generated SpikeTrain objects are cached in self._items
There was a problem hiding this comment.
Yes, you are right. So in this PR the SpikeTrain representation is still the reference all data will be converted to eventually. Even when merging two SpikeTrainLists coming from a spike_time_array representation. Is there any way we can avoid that? Maybe having a default operation mode that can switch between SpikeTrain and spike_time_list? Or would it be worth to have the _spike_time_array and the corresponding masks for individual channels / units as base representation and only generate SpikeTrains when required (without) caching / duplicating the data? This would rely even stronger on numpy for inserting / reordering of the spike_times lists and masks. I am not sure which method would be more performant in the end. @apdavison Do you have a better intuition there?
There was a problem hiding this comment.
you're right that merging two array-representation SpikeTrainLists should avoid creating SpikeTrains. I think that's easy enough to do.
The general idea is to keep data in the representation they arrive in as long as possible, to avoid unecessary transformations, i.e. the "reference/base" representation depends on how the object was initialized.
Note that the multiplexed property (not yet implemented in this branch, but you can see it in the spiketrainlist branch which will replace this PR) allows users to access the array representation.
| else: | ||
| self._items.extend(other._items) | ||
| return self | ||
| elif other and isinstance(other[0], SpikeTrain): |
There was a problem hiding this comment.
In principle this would need to check if other is iterable and assert that all contained items are SpikeTrain instances, no?
There was a problem hiding this comment.
yes, this was a shortcut, but it would be more robust to do a full check
| self._spiketrains_from_array() | ||
| for obj in iterable: | ||
| obj.segment = self.segment | ||
| self._items.extend(iterable) |
There was a problem hiding this comment.
maybe it's necessary to also verify that the items of iterable are instances of SpikeTrain? Otherwise arbitrary objects might be in the SpikeTrainList...
There was a problem hiding this comment.
yes, more robust checks are needed
| } | ||
| return obj | ||
|
|
||
| def _spiketrains_from_array(self): |
There was a problem hiding this comment.
Yes, you are right. So in this PR the SpikeTrain representation is still the reference all data will be converted to eventually. Even when merging two SpikeTrainLists coming from a spike_time_array representation. Is there any way we can avoid that? Maybe having a default operation mode that can switch between SpikeTrain and spike_time_list? Or would it be worth to have the _spike_time_array and the corresponding masks for individual channels / units as base representation and only generate SpikeTrains when required (without) caching / duplicating the data? This would rely even stronger on numpy for inserting / reordering of the spike_times lists and masks. I am not sure which method would be more performant in the end. @apdavison Do you have a better intuition there?
|
SpikeTrainList now implemented in #1000. Closing this now. |
4 participants
As discussed in #125, the parent-child relationships between objects are currently handled manually, e.g. when you add a
SpikeTrainto thesegment.spiketrainslist, the reverse relationshipspiketrain.segmentis not automatically created, leading to potential inconsistencies.The aim of this PR, when complete, is to automatically create these relationships by replacing the various child lists (
Segment.spiketrains,Segment.analogsignals,Block.segments, etc) by pseudo-list objects, and the reverse relationshipsSegment.block, etc., by properties).The pseudo-list classes also provide the opportunity for additional logic and optimisations, for example, a multiplexed representation of spike trains (all times in a single array, with a second array indicating which neuron/channel the spike is from) which will be much more efficient for data coming from simulators.
child.parentpropertiesComments welcome!