[WIP] Make AnalogSignal and waveform dimensions more consistent

[JuliaSprenger]

This PR changes the order of the dimensions for waveforms from (spike, channel, time) to (time,spike,channel) in order to be consistent with neo AnalogSignal dimensions (time, channel).

Neo Core and corresponding tests were updated, updates in the IO are still missing.

[pep8speaks]

Hello @JuliaSprenger! Thanks for updating the PR.

• In the file neo/io/brainwaresrcio.py, following are the PEP8 issues :

Line 297:13: E722 do not use bare except'
Line 356:13: E722 do not use bare except'

• In the file neo/rawio/blackrockrawio.py, following are the PEP8 issues :

Line 1507:28: E127 continuation line over-indented for visual indent
Line 1508:28: E127 continuation line over-indented for visual indent

• In the file neo/rawio/plexonrawio.py, following are the PEP8 issues :

Line 101:75: E502 the backslash is redundant between brackets
Line 122:72: E502 the backslash is redundant between brackets

• In the file neo/rawio/spike2rawio.py, following are the PEP8 issues :

Line 87:72: E502 the backslash is redundant between brackets
Line 103:29: E128 continuation line under-indented for visual indent
Line 122:28: E225 missing whitespace around operator
Line 125:100: E501 line too long (100 > 99 characters)
Line 126:67: E225 missing whitespace around operator
Line 142:69: E225 missing whitespace around operator
Line 149:40: E225 missing whitespace around operator
Line 150:29: E127 continuation line over-indented for visual indent
Line 159:31: E225 missing whitespace around operator
Line 165:31: E225 missing whitespace around operator
Line 180:100: E501 line too long (101 > 99 characters)
Line 359:30: E225 missing whitespace around operator
Line 370:5: E303 too many blank lines (2)
Line 391:33: E128 continuation line under-indented for visual indent
Line 450:41: E128 continuation line under-indented for visual indent
Line 462:13: E128 continuation line under-indented for visual indent
Line 481:41: E128 continuation line under-indented for visual indent
Line 481:100: E501 line too long (109 > 99 characters)
Line 502:17: E128 continuation line under-indented for visual indent
Line 505:17: E128 continuation line under-indented for visual indent
Line 508:37: E128 continuation line under-indented for visual indent

• In the file neo/test/generate_datasets.py, following are the PEP8 issues :

Line 366:17: E129 visually indented line with same indent as next logical line

• In the file neo/test/iotest/test_blackrockio.py, following are the PEP8 issues :

Line 235:32: E231 missing whitespace after ','
Line 235:38: E261 at least two spaces before inline comment

• In the file neo/test/iotest/test_neuralynxio.py, following are the PEP8 issues :

Line 270:13: E122 continuation line missing indentation or outdented

Comment last updated on March 06, 2018 at 08:59 Hours UTC

[coveralls]

Coverage decreased (-0.009%) to 48.805% when pulling e40bb47 on INM-6:switchwaveformdims into 30bdf56 on NeuralEnsemble:master.

[cboulay]

As numpy and scipy are by default row-major (the last dimension is what changes the fastest; their functions default to operating on axis=-1), and the majority of signal processing steps that will be done on these types of signals will be performed down the time dimension, I think it makes more sense to align AnalogSignal with waveforms, i.e. (channel, time), than vice versa.

Here is a gist that demonstrates this. In this case there was only a 2x speedup by operating on the last axis instead of the first and I'm sure I've encountered situations where the difference was greater than that.

[apdavison]

@cboulay The decision to have time on axis 0 was made very early in the history of Neo, motivated by an EEG use case, if I remember correctly.

I would be in favour of changing this at some point, but it would be a big change, and would break all downstream code built on Neo. I would be inclined to do this for Neo 2.0 (Neo 1.0 is planned for later this year, maybe next year).

[cboulay]

Changing the waveform axes is also an API break, isn't it? But I understand that changing the AnalogSignal dimensions is a 'bigger break' than the waveforms, because fewer applications use the waveforms.

Alternatively, you can keep the axis ordering as is but change to 'F' ordering in the memory layout. This would keep the API the same but you'd get the speedup benefit now.

[samuelgarcia]

I am really in favor of keeping time as dim 0 because this the natural data order for any recording system of a multi channel signal.

If you want to walk through really big files of long recordings having channels as first dim is just not possible.

Of course when you you do a by channel computation you will get better performence if channel is dim 0 because each channel is compact in memory.

For instance in tridesclous and ephyviewer I really need a chunk in time access very fastly. I guess that many of us will need that. In that case the time = dim 0 wins.

Furthermore the rawio level expose data as time (time, channel) because they amost always are (time, channel) inside the file so a swap in memory would lead to bad performence at the reading time.

I think if someone really need (channel, time) in memory layout. We can do something like this:

anasig = AnalogSignal(...)
sig = anasig.magnitude
sig_t = sig.T.copy().T
anasig_t = AnalogSignal(sig_t)

In this example anasig_t have internally in memory a reverse stride but keep time dim = 0.

[samuelgarcia]

I am OK with this PR.

Could you run the cricle-ci run again to tests what need to be change in all IOs ?

[samuelgarcia]

Hi Julia.
I am not surelike to like so much changing waveforms dim order at rawio level with np.moveaxis .
I would have done this np.moveaxis only in BaseFromraw but not in each rawio.

Pro:

• neo.io and neo.rawio are consistent for wavefroms dimension.

But:

• it break the underlying memmap of some files that are (n_spike, sample, n_channel) which is the natural order for a recording system that stack some snipet of multichannel interleaved.

In my mind rawio is more focus on performence that consistency.

I practice, if I am not wrong, it is not a big problem because np.moveaxis only change strides and do not make a full memory transpose (this would be very bad for performences).

Could you check a change of perf with this moveaxis ?

[JuliaSprenger]

Hi Samuel,
I also think that the moveaxis operation should not cost much of performanoce all new io to follow a different dimension ordering in the raw io than in the actual io. I think this might be confusing. Also for some IOs you might need to swap axis at the rawio level to counteract the moveaxis at basefromraw level, which I does not make sense in the long run.

[samuelgarcia]

OK.
So keep the moveaxis in each rawio.

But we need to keep in mind that (sample, channel, spike_num) order penalize memory acces because if I want to plot one waveforms for one spike all sample will be spreed over the memory block. With high spike count it could affect the speed.
Of course this is not the case with moveaxis on raw buffer but if the user want the rescaled to units (uV) wavefroms (the default at neo.io) then smaple of the same wavefroms are spread over memory.

Anyway, lets go heahed and we will se if performence are not good.

[JuliaSprenger]

Circle tests are passing, only travis with python 3.4 is from time to time failing, but the reason seems not to be related to my changes. Restarting Travis helped. I reformatted to get rid of the majority of pep8 complaints, but the remaining ones are to be fixed in more io specific PR and not here.

[samuelgarcia]

@JuliaSprenger : do you still plan this for neo 0.7 or can we postone to 0.8 ?

[JuliaSprenger]

@samuelgarcia @apdavison I think this PR won't be merged any more as we should first agree on a solution concept in #829.