Replace ChannelIndex and Unit by View and Group #817
Conversation
|
Hello @apdavison! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-11-06 12:41:05 UTC |
|
Thanks, Andrew, I will go over this PR over the next days. I don't have an ad-hoc opinion on whether to keep or drop the old classes yet. |
|
Hi Andrew. I also try to have a look before the end of the week end. After everyone in core team have read this, maybe we could plan a very short call to accelerate discussion, what do you think ? |
|
I will start to read your PR. Some feeling right now (before reading) :
|
|
I haven't had a good look at the PR but I skimmed through it yesterday. I agree with Samuel that it might not be that difficult to read old NIX files into the new format. I think an important part of the decision is whether the user or the IO should control the old-to-new conversion.
I suppose in the latter case it could be accompanied by a warning informing the user that "N ChannelIndex objects have been converted to View". Then it's up to the user to create a different kind of object if they need to (assuming View isn't the only replacement type). That's just some initial thoughts I had on the decision. I understand why you would want to avoid having the complexity of keeping around the old object types alongside the new ones, especially if it might lead to users mixing old with new classes while they work with the transition version. |
| for i, spiketrain in enumerate(spiketrains): | ||
| unit = Group(spiketrain, name=f"Neuron #{i + 1}") | ||
| units.append(unit) | ||
|
|
There was a problem hiding this comment.
I think this example would more clear with sevral segment.
a unit is more easy to understand across segment. No ?
There was a problem hiding this comment.
This example is just a direct translation of the previous one, it's probably not the best example for the new features.
We should write new examples when we rewrite the documentation.
| segment.spiketrains.append(spiketrain) | ||
| spiketrain.segment = segment | ||
| # assign each spiketrain to a given neuron | ||
| current_group = next(iter_group) |
There was a problem hiding this comment.
I like this example.
Maybe the use of next.cycle is a bit confusion for a python beginner.
Maybe an old school and less elegant indexing would be more explicit.
No ?
There was a problem hiding this comment.
yes, but I propose to leave it for now since we plan to rewrite the documentation (and examples) anyway.
neo/core/group.py
Outdated
| _container_child_objects = ('Segment', 'Group') | ||
| _single_parent_objects = ('Block',) | ||
|
|
||
| def __init__(self, *objects, name=None, description=None, file_origin=None, **annotations): |
There was a problem hiding this comment.
Maybe, I am old school but I would expect objects an iterable and not *objects here.
A standard use case will be
a_list = []
for i in myloop:
a_list.append(an_obbect)
# This is more intuitive a biginer
g = Group(a_list)
# if I understand correctly you propose this
g = Group(*a_list)
neo/core/view.py
Outdated
| if self.index.dtype == np.bool: # convert boolean mask to integer index | ||
| if self.index.size != self.obj.shape[-1]: | ||
| raise ValueError("index size does not match number of channels in signal") | ||
| self.index = np.arange(self.obj.shape[-1])[self.index] |
There was a problem hiding this comment.
or self.index, = np.nonzero(self.index)
|
Hi andrew. My main feelings: Group View Let Julia, Michael, Achilleas have a look and we could maybe plan a short meeting, no ? |
|
@apdavison Thanks for going ahead and starting this! Regarding the deprecation of Therefore I think we should provide a function to convert any given neo structure based on Besides these thoughts regarding |
There was a problem hiding this comment.
fixing the issues in #456 comes at the cost of introducing more abstractions that you call Group and View. I cannot judge how hard things were in the past with ChannelIndex, but I find these changes useful. Here are my suggestions:
- the name "View" is very ambiguous; I'd rename it to AnalogSignalView or at least SignalView;
- before I came upon "the Unit class has been deprecated in favor of Group", it was confusing to read
unit = Group(). Then I thought OK, Group is basically grouping by a unit id. I'd rename Group to UnitGroup or GroupedByUnit (like the result of a pandasdataframe.group_by()) because essentially what you call Group is NOT a physical identity but a result of the analysis that neuron's activations, something physical and yet unobserved, are diluted into several channels. You call it Group as if it's another physical identity like Segment or Block. It's rather a mask (but please don't call it Mask). - one way to build these relationships is by hand (by user), another - an automatic call, something like
segment.to_groups(), assuming that each AnalogSignal and SpikeTrain are somehow already linked to channel indices. This will remove the burden for users of switching to the new API if you leave ChannelIndex deprecated instead of complete removal.
Consider another example. An analog signal has information in its annotations that it belongs to channel "2" (is there such kind of annotations available?), and there is a spike train with the annotation that it also belongs to channel "2". It'd be naturally then to split a segment into a list of UnitGroups transparently for the user just by parsing the annotations for each analog signal and spike train.
| None | ||
|
|
||
| *Recommended attributes/properties*: | ||
| :objects: (Neo object) Objects with which to pre-populate the :class:`Group` |
There was a problem hiding this comment.
need to specify of which type: only AnalogSignal and SpikeTrain, right?
There was a problem hiding this comment.
no, a Group can contain any Neo object.
|
I've made the agreed changes. What do you think about "ChannelView" instead of "View"? |
|
@apdavison : I think that I am able to push some change into your branch. Is it OK for you ? |
|
I suggest to start a list of practical todos here, so we keep an overview of what is missing before this PR can be merged:
import neo
import quantities as pq
# creating neo structure with one instance of each object type required
bl = neo.Block(name='1')
bl.groups.append(neo.Group(name='2', allowed_types=[neo.ChannelView]))
anasig=neo.AnalogSignal([1,2,3],units=pq.dimensionless, sampling_rate=1*pq.Hz, name='4')
bl.groups[0].add(neo.ChannelView(anasig, [0], name='3'))
# segment is required for nixio to save the dataobjects first
bl.segments.append(neo.Segment(name='5'))
bl.segments[0].analogsignals.append(anasig)
# create a 2nd group to reference the same ChannelView
bl.groups.append(neo.Group(name=6, allowed_types=[neo.ChannelView], objects=bl.groups[0].channelviews))
io = neo.NixIO('testfile.nix', 'ow')
io.write_block(bl)
io.close()Feel free to extend this list. |
Add ChannelIndex and Unit to ChannelView and Group converter
|
Hi all. |
Still have error due to asyymetric relationship in group but I think we need to fix unitest.
|
A question about "Represent units as groups (BasefromrawIO)" for @samuelgarcia and @JuliaSprenger : In the master branch, in i.e. raw IOs do not support use-case 2 for ChannelIndex
Is that correct? Don't any of the file formats contain the information necessary for this association? |
I would hope that this association is possible via the channel ids. Or are there systems that use different channel_ids for continuously sampled data and spiketrain data coming from the same electrode? What do you think @samuelgarcia? |
|
@apdavison : ChannelIndex was more an object to reconstruct this with spike sorting tools but quasi impossible to handle at file reader level (or too much work). In my opinion this relationship between a SpikeTrain and some channel is less and less usefull in the spike sorting work with dense probe. In short this object can be usefull but it is not a raw input from the data. |
# Conflicts: # neo/core/analogsignal.py # neo/io/basefromrawio.py # neo/io/nixio.py # neo/test/coretest/test_analogsignal.py # neo/test/iotest/test_axographio.py # neo/test/iotest/test_exampleio.py
|
thanks @samuelgarcia and @JuliaSprenger so it seems that rawio has never had full support for Unit. In that case, I think the new basefromrawio that Samuel wrote is essentially feature-equivalent with the previous version. We can try to add more functionality, but perhaps after the release. I've now fixed the conflicts with master, and the tests seem to be passing, so I think this is ready to merge. I suggest we merge this now, and then make any remaining changes that are needed before the release with PRs onto master. |
apdavison
changed the title
|
Hi @samuelgarcia & @apdavison. Ok, then we postpone the unit-channel linking enhancement discussion to another release and I will merge this PR now. Thanks for the amount of work put into the new concept. I hope this is the last structural change we need for representing channel relations. |
|
Champagne!!! |
7 participants
The goal of this is to complete the implementation of #456. Please take a look, and let me know if this fits your understanding of what was agreed, and if you see any difficulties with this approach.
Most of the necessary changes to
coreare done, I think,iohas not been touched yet.An important decision is whether we keep ChannelIndex and Unit for one major release, but deprecate them, or whether we remove them in the same release where we add View/Group. The advantages of keeping them are that (i) people can test the old and new approaches side-by-side, (ii) existing files in NIX format can be read, converted by the user, and rewritten with the new objects (otherwise NIXIO will have to be convert things automatically, which may not be easy). The disadvantages are the increased complexity and possible confusion of having two ways to do the same thing in Neo at the same time.