Add concatenate functionality for signal objects

[JuliaSprenger]

This PR adds patch methods for AnalogSignals and IrregularlySampledSignals. Patching allows to combine signals along the time axis, so potentially across different Segments. The behaviour for IrregularlySampledSignal and AnalogSignals differs slightly, as for AnalogSignals overlapping sampling points are not possible in contrast to IrregularlySampledSignals, where all samples can just be combined irrespective of temporal overlaps. For AnalogSignals, the prioritized sampled can be specified using the overwrite keyword.

This feature can be for AnalogSignals extended to also handle padding during patching, therefore providing a solution to #820.

[pep8speaks]

Hello @JuliaSprenger! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file neo/core/analogsignal.py:

Line 764:100: E501 line too long (100 > 99 characters)

Comment last updated at 2020-11-09 13:30:19 UTC

[samuelgarcia]

Hi julia.
Good idea.
I don't known if "patch" is the good naming.
maybe "extend" ?

[apdavison]

I agree with Samuel that "patch" is not the best name, it implies replacing part of the existing signal (which is already done by the "splice" method.

"extend" or "append" would be better.

I also think it's confusing to have different behaviour for the two types of signal. I suggest restricting the method for IrregularlySampledSignals to forbid overlap.

[JuliaSprenger]

Let's go for concatenate as method name.

[JuliaSprenger]

As suggested by @apdavison, overlapping IrregularlySampledSignals can now not be concatenated but raise a ValueError. However, since I believe there might be cases when concatenating such signals might make sense anyway, I introduced a keyword for this to allow merging or overlapping signals if explicitely requested.

[JuliaSprenger]

Hi @morales-gregorio, could you please check if this implementation fulfils the criteria you were trying to adress in #846 ?

[morales-gregorio]

Hi @JuliaSprenger, thanks for this nice function!

As far as I understood with this function you cover the concatenation across one axis (time) and the function I wrote in #846 was meant to 'stack' signals across electrodes.

Do you prefer to have two separate functions for each axis or just one function? I personally think having it all under the concatenate function is the cleanest, and I would (1000 times) go with yours ;) It would further require something like an axis kwarg and some checks for the length of the signals (maybe some of the checks I already implemented in #864 could be copy-pasted here).

I am happy to incorporate the changes if you want to. Also, let me know if you want a more thorough line-by-line code review, which I am happy to do :)

Best,
Aitor

PS I really liked the padding idea :)

[JuliaSprenger]

@morales-gregorio Thanks for having a look!
Yes, this function is only for concatenation along time.
I think that 'concatenation' along different axis has very different implications within the neo framework justifying implementation in different functions. E.g. 'padding' is only applicable for concatenation along time, but not across channels and the set of attributes that need to match is also very different, see also #846 (comment) . Also two separate functions for this makes the user interface more explicit and the users more aware of what they are doing in terms of data manipulation. @samuelgarcia Do you share this opinion?
The corresponding function to use for 'concatenation' across channels would be the merge

python-neo/neo/core/basesignal.py

Line 231 in 6813416

def merge(self, other):

@morales-gregorio I would be interested if the performance of my implementation can keep up with your version in your use case, since this was the original motivation for your work. If you also require performance improvements for the merge let me know and I will have a look at it.

And yes, it definitely makes sense to also add some of the tests you implemented here. I will have a look at it.

[JuliaSprenger]

@samuelgarcia @apdavison I would propose to merge this soon if you agree with the API and add potential performance optimizations later on when @morales-gregorio had time to run performance checks on his side.

[JuliaSprenger]

The failing tests are related to the new h5py version, fixed in #893.

[samuelgarcia]

This looks good.
Tests looks really exhaustive. Cool.

I have some questions/comments:

• when merging how do we deal with the sample missalignement ? In short do we allow
  merging two signals that have an epsilon clock shift ? For instance at 10Hz t_start =
  0.0 and t_start=10.02 espilon is 0.02
  Is it done at splice level ?
• I don't remember when the MergeError was introduced. I think this error is a bit too
  specific.
• shall we check that the dtype is constistent across the inputs ? And so add it in
  "shared_attributes" ? Here you take the first signal dtype.
• I don't known when the union operator (&) on dict was introduced in python.
  Is compatible with our release policy ? It is used in interset_annotation.

[JuliaSprenger]

Hi @samuelgarcia. Thanks for the review.

• Sample missalignment is handled via the splice method, that is finding the closest matching t_start for the signal to be merged via the AnalogSignal.time_index() method. This might lead to shiifts up to the size of sampling_period. I added a note in the documentation.
• The MergeError is already used in the merge method and did not lead to version conflicts so far. For consistency I would also stick to the MergeError here. Or do you want to remove it also in the merge() method?
• I am not sure adding the dtype consistency check would do more help or harm. I can imagine cases where this can lead to frustrating situations on the user side when setting the dtype of all signals explicitly before being able to concatentate. I think we should either find a way of cleverly selecting the dtype that matches all signals or just go with the dtype of the first signal (as currently) and thereby stick to the default numpy behaviour.
• I now converted to sets, which are supporting the intersection operator '&' for some time already.

[samuelgarcia]

Perfect.
It is OK for me.
@apdavison : will you have time to review this this ? or do I merge it now ?

[JuliaSprenger]

Once the tests passed, feel free to hit 'merge' :)

[apdavison]

merge when ready!