[apdavison]

Fixes #1935: several bugs in ObjectList (and its subclass SpikeTrainList) around adding objects, membership tests and validation.

Two behaviour changes may surprise anyone relying on the current behaviour:

• in, index(), count() and remove() now compare by identity rather than equality, so a different object with equal data is no longer matched.
• Assigning anything other than a list or ObjectList to a container's child list (e.g. seg.analogsignals = (sig,)) now raises TypeError instead of being silently ignored.

New tests in neo/test/coretest/test_objectlist.py, test_spiketrainlist.py and test_container.py; 20 of them fail on master.

[zm711]

Just glancing this looks great! I'll do a careful read over this tomorrow.

[viranovskaya]

I found one remaining inconsistency in the eager SpikeTrainList += SpikeTrainList path on dcc73a0:

import neo
import quantities as pq
from neo.core.spiketrainlist import SpikeTrainList

st = neo.SpikeTrain([1] * pq.ms, t_stop=10 * pq.ms)
items = SpikeTrainList([st])
items += SpikeTrainList([st])
print(len(items), items.count(st))

This prints 2 1 on this PR, versus 2 2 on base 442658a.

__iadd__ calls _add_spiketrainlists(..., in_place=True), which extends _items directly and bypasses the duplicate checks in extend(). The new ObjectList.count() therefore returns 1 even though the same object is stored twice.

If duplicate identities are meant to be rejected here too, this path needs validation before modifying the list. Otherwise, count() still needs to count all stored identity matches. Could this combination be covered by a regression test?

[zm711]

I see that @viranovskaya.

So here do _items.extend() instead of doing self.extend so we don’t verify that we aren’t adding duplicates. That seems like a fair change to make from my perspective @apdavison.

python-neo/neo/core/spiketrainlist.py

Lines 178 to 189 in dcc73a0

	if self._spike_time_array is None or other._spike_time_array is None:
	# if either self or other is not storing multiplexed spike trains
	# we combine them using the list of SpikeTrains representation
	if self._items is None:
	self._spiketrains_from_array()
	if other._items is None:
	other._spiketrains_from_array()
	if in_place:
	self._items.extend(other._items)
	return self
	else:
	return self.__class__(items=self._items[:] + other._items)

[apdavison]

Thanks @viranovskaya, good catch. Several other SpikeTrainList paths had the same problem: stl + other_stl, stl + [st], [st] + stl and the constructor itself (SpikeTrainList([st, st])). 533d7c2 routes all of them through the same duplicate check, with regression tests for each.

Routing stl += other_stl through extend() exposed that extend() was O(n.m), so 66984c3 makes it O(n + m). Timings for stl += other_stl, where both lists contain n SpikeTrain objects:

n	dcc73a0 (no check)	533d7c2	66984c3
2000	<0.1 ms	49 ms	0.4 ms
10000	<0.1 ms	1.2 s	2.4 ms
50000	0.4 ms	38 s	18 ms

The multiplexed representation is unaffected: += between two multiplexed lists still takes the array path (0.3 ms for 10,000 channels) and stays multiplexed.

[zm711]

This looks good to me now. I'll leave it open until Monday in case anyone else wants to give it a read unless you wanted it merged sooner @apdavison.