improves blackrock spikes in nev #1152
Conversation
|
Hello @ferchaure! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2022-08-30 15:33:56 UTC |
| waveforms = unit_spikes['waveform'].flatten().view(wf_dtype) | ||
| dt1 = [ | ||
| ('extra', 'S{}'.format(unit_spikes['waveform'].dtype.itemsize - wf_byte_size)), | ||
| ('ch_waveform', 'S{}'.format(wf_byte_size))] |
There was a problem hiding this comment.
How did you get this specification of extra bytes ? is it in the official documents ?
There was a problem hiding this comment.
Not really, the code looked like it follows the documentation, but I saw that the available bytes for the waveforms are bigger that the ones expected given the dtype. Maybe the reserved bytes ( the ones before the waveform data) must be dynamic and not always uint8 (but I couldn't find documentation to support that theory). For that reason I did that dt1 type, just to extract the expected bytes
There was a problem hiding this comment.
But how we can be sure that extra bytes are before ?
And what if extra size == 0 ?
There was a problem hiding this comment.
good question, I don't know, I just tested it in a file and there is the relationship with the reserved bytes that are before. (I can share the file if you want)
I checked the case when extra size == 0 and it's fine, it just creates an empty binary field without errors
|
The last commit edits the segment stop just to extract spikes times (adding a sample to let it work fine with the stop of a slice), it fixes the same without editing the _seg_t_stops attribute |
There was a problem hiding this comment.
Merged with the current master branch, I was able to successfully load two of our v2.3 data files, containing 2 and 3 segments, respectively. Regarding the waveforms, one had 38, one 48 samples, both was correctly loaded with an extra field of size 0 in dt1. Regarding the missing spikes, I can confirm that with the patch, in one of the two data files 3 additional spikes were loaded which are sitting exactly on t_stop, i.e., the end of the segment. These spikes were missed with the previous version of the IO, and I agree that the user would expect these spikes to be included instead.
Looking at the code, it looks good to me. Thanks for the patch, very useful!
|
Decision was made during a Neo discussion to add a flag to the loader that can be used to select the alternate waveform loading. |
Hi,
I tried to open a .nev file (2.3 version) and I found t2o bugs and fix them:
-The timestamps gives one element less that the amount of spikes. I follow that issue to the _seg_t_stops definition and how is used to create a slice. Before the highest element in the nev file was defined as the t_stops and used to slice... keeping the last element outside the range. I just add a sample before converting to time and the appending to _seg_t_stops
-It wasn't possible to load the waveforms. The reshape in _get_spike_raw_waveforms throw an error related to sizes mismatch. In a few words, it had to many bytes for the waveforms (in my case 96 when each waveform should have 92). I could increase the bytes for the reserved fields of the Spikes fields in the __nev_data_types ... but given that each channels has its own waveform_dtypes maybe it has to be resize for each case. For that reason I create that additional dt1 type, to remove the extra bytes knowing how many bytes should be there for the waveform.
Cheers,
Fernando