Split function and new_unit_ids definition #2694

[OlivierPeron]

Change discussed in issue #2694

[zm711]

Suggested change

	indices_list = [indices_list]
	indices_list = list(indices_list)

Is this the right solution? for example if I do:

indices_list = np.array([1,2,3])

this will turn this into indices_list = [array([1,2,3])] instead of indices_list = [1,2,3]

whereas using the list constructor will work correctly.

[OlivierPeron]

What we want is something like indices_list = [array([1,2,3])], with one array per recording' segment. If you have indices_list = [1,2,3] then this max([v.max() for v in indices_list]) + 1 could raise an AttributeError: 'int' object has no attribute 'max' if v is int instead of np.int, but anyway the point is that v should be an array and not a value.

[alejoe91]

why not just doing np.max(v) instead ov v.max()? the former works both for lists and array.

Regarding the checks, we should also check that indices_list[0] is an iterable, so I would do this:

from collections.abc import Iterable

...

if not isinstance(indices_list, list):
    indices_list = [indices_list]
if not isinstance(indices_list[0], Iterable):
    raise ValueError(...)
new_unit_ids = self._get_unused_id(np.max([np.max(v) for v in indices_list]) + 1)

This protects from passing indices_list=[1,2,3], which should fail in my opinion. Instead, passing np.array([1,2,3]) or [[1,2,3]] should work.

[zm711]

This protects from passing indices_list=[1,2,3], which should fail in my opinion. Instead, passing np.array([1,2,3]) or [[1,2,3]] should work.

Exactly. Because we were just checking list I was confused. I also think we need a docstring in this case. Needing a list of a list or an array is not intuitive for a variable named indices_list. With a docstring at least the user can be warned what is expected.

[alejoe91]

Closing in favor of #2775