include analog signal merging utility function and the corresponding … #846
Conversation
…test, which passes in my machine
|
Hello @morales-gregorio! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-08-26 11:16:05 UTC |
| if not anasiglist[0].t_start == anasig.t_start: | ||
| raise ValueError('t_start must be the same for all signals') | ||
| if axis == 0: | ||
| if not anasiglist[0].magnitude.shape[1] == \ |
There was a problem hiding this comment.
The number of channels is represented by the last dimension, so better check anasiglist[0].magnitude.shape[-1] here and below
| if axis == 0: | ||
| if not anasiglist[0].magnitude.shape[1] == \ | ||
| anasig.magnitude.shape[1]: | ||
| raise ValueError('Analogsignals appear to contain different ' |
There was a problem hiding this comment.
...then you can also remove the appear here.
| anasig.magnitude.shape[1]: | ||
| raise ValueError('Analogsignals appear to contain different ' | ||
| 'number of channels!') | ||
| if axis == 1: |
There was a problem hiding this comment.
this should accordingly check all other dimensions except for the first one as in principle there could be more than 2.
|
I think there's some common features to PR #836 and this one. I will have a look, maybe we can unify the functionality somehow. |
| assert_same_annotations) | ||
|
|
||
| from neo.utils import (get_events, get_epochs, add_epoch, match_events, cut_block_by_epochs) | ||
| from neo.utils import (get_events, get_epochs, add_epoch, match_events, |
There was a problem hiding this comment.
I don't think this change is needed since Neo allows for up to 99 characters per line, see
Line 2 in 57f8d07
Maybe you are using the default PEP8 settings for autoformatting here?
| list of analogsignals that will be merged | ||
| axis: int | ||
| axis along which to perform the merging | ||
| `axis = 1` corresponds to stacking the analogsignals |
There was a problem hiding this comment.
By implementing the combination of signals along two different axis you are covering two different scenarios, that require different checks on the neo level. Eg. when stacking signals (ie merging multiple channels in one Analogsignal) it is useful to ensure they have the same length and cover the same time range. However for concatenating analogsignals along time, these conditions do not need to be fulfilled. I would argue therefore these different cases should also be implemented in separate functions. This would also make it easier for the users to decide which of the actions they actually want to use.
There was a problem hiding this comment.
Another feature that needs to be treated differently depending on the axis are the array_annotations. I am not sure in how far this implementation is taking care of this, but I think we should double check here.
| if axis == 1: | ||
| if not anasiglist[0].magnitude.shape[0] == \ | ||
| anasig.magnitude.shape[0]: | ||
| raise ValueError('Cannot merge signals of different length.') |
There was a problem hiding this comment.
A reoccurring scenario is that some data can only be loaded into neo with try_signal_grouping=False because of a single sampling point missing in some of the signals. In this case it would be useful to have a force_merge option which ignores the different lengths and cuts the signals to the shortest signal length.
| for anasig in anasiglist: | ||
| anasig0.array_annotations = anasig0._merge_array_annotations(anasig) | ||
|
|
||
| array_annot = anasig0.array_annotations |
There was a problem hiding this comment.
It seems that there is a distinction missing whether the analogsignals are concatenated or stacked, which would require different handling of the array annotations.
I guess the protocol should be something like:
- concatenating: keeping only array_annotations and annotations that are present and equal in all analogsignals in the list, while omitting (?) the rest.
- stacking: stacking the array_annotatations; keeping only the annotations that are present and equal in all analogsignals; annotations that are different for the analogsignal should then be used as array_annotations instead.
or what do you think is a good merging strategy?
|
Hi @morales-gregorio, |
|
The performance issues that motivated this PR should have been addressed in #836. Please re-open, or create a new issue, if the performance issues remain. |
6 participants
Hey!
I finally managed to create the PR for issue #830
Please have a look if you think something is wrong or should be improved.
Best,
Aitor