Spikeglx np2 #1045

samuelgarcia · 2021-10-29T11:26:24Z

Support for neuropixel 2.0
clean
remove location (because wring in neo) use prointerface for correct channel location
add load_sync_channel option to not load the last channel

See #1028

…in probeinterface.

pep8speaks · 2021-10-29T11:26:27Z

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

In the file neo/rawio/spikeglxrawio.py:

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

In the file neo/test/rawiotest/test_spikeglxrawio.py:

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

Comment last updated at 2022-01-19 17:30:03 UTC

samuelgarcia · 2021-11-10T12:38:20Z

@JuliaSprenger : could you merge this https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/61 so I can run test for this ?
Thank a lot.

samuelgarcia · 2021-11-10T12:44:12Z

@JuliaSprenger : ready for review.

JuliaSprenger · 2021-11-10T13:24:22Z

@JuliaSprenger : could you merge this https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/61 so I can run test for this ? Thank a lot.

Done, the test files are merged and io tests are restarted.

samuelgarcia · 2021-11-10T13:33:25Z

cool thanks

JuliaSprenger · 2021-11-10T15:56:37Z

neo/rawio/spikeglxrawio.py

-                        shape=(info['sample_length'], info['num_chan']), offset=0, order='C')
+            data = np.memmap(info['bin_file'], dtype='int16', mode='r', offset=0, order='C')
+            # this should be (info['sample_length'], info['num_chan'])
+            # be some file are shorten


shouldn't this then maybe raise a warning if the info metadata and the data length don't match?

@samuelgarcia What is your opinion about this?

neo/rawio/spikeglxrawio.py

JuliaSprenger · 2021-11-10T16:21:08Z

Hi @samuelgarcia! I don't agree with removing the channel_location array_annotations, as 1) this might be used already and removing it will break existing code and 2) afaik there's no instructions on how to create a corresponding probeinterface object from a spikeglx dataset like this one.

JuliaSprenger · 2021-11-10T16:22:51Z

The failing tests seem to be not related to your changes, but to nix / h5py.

samuelgarcia · 2021-11-10T16:38:00Z

Channel location was wrong in neo.
So unless we have a dependency on probeinterface to load the channel map. I don't think think it is good to maintain channel location neo because is more complicated what I did before.

samuelgarcia · 2021-11-15T08:06:03Z

Hi Julia.
About the channel_location. I don't think it is relevant to maintain code here for channel location.
Is was wrong on neo side and well maintained on probeinterface side.
Furthermore, the code in probeinterface is growing because handling more version.

I don't think channel_location has been used with neo. Otehrwise we would have get some issues.

So 3 solutions:

remove the channel location here
keep it but with a hard dependency on probeinterface
keep it but with a soft dependency on probeinterface with an option (load_channel_location)

JuliaSprenger · 2021-11-16T14:19:15Z

Hi @samuelgarcia,
I was more wondering about channel_location already being used on the user side, not within neo or other packages.
What I am missing is the corresponding way on how to access the same data using probeinterface.
Do you have the plan of adding probeinterface as a dependency of neo? Otherwise I would go for a weak dependency, as only this IO is affected.

samuelgarcia · 2021-11-16T14:55:01Z

No I don't think we should add probeinterface as a dependency for neo.
weak dep would be ok in that case.

To add the location is super easy

# read signals
glx_folder = '/bla/bla'
reader = neo.SpikeGLXIO(dirname=glx_folder)
anasig = reader.read_segment().analogsignals[0]
# read probe
probe = probeinterface.read_spikeglx(glx_folder / 'blabla.meta')
# set location in annotations
anasig.array_annotations['channel_location'] = probe.contact_positions

This is why I wanted to remove location handling from neo to avoid maintenance because it is already maintain in PI.
Add weakref and doing theses lines internally is also possible.
Making user dealing directly with probeinterface is also good...

neo/test/rawiotest/test_spikeglxrawio.py

luiztauffer · 2022-01-07T12:31:00Z

hey @samuelgarcia , I'm trying to use this PR code, getting this error when reading:

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-2-5bd6546cdd03> in <module>
      2 meta = str(dir_path / "m676p3l11#1_ori16_g0_t0.imec.ap.meta")
      3 
----> 4 rec = se.SpikeGLXRecordingExtractor(folder_path=dir_path, stream_id='imec.ap')
      5 # probe = read_spikeglx(file=meta)
      6 # rp = rec.set_probe(probe=probe)

/media/luiz/storage/Github/spikeinterface/spikeinterface/extractors/neoextractors/spikeglx.py in __init__(self, folder_path, stream_id)
     35         if HAS_NEO_11:
     36             neo_kwargs['load_sync_channel'] = False
---> 37         NeoBaseRecordingExtractor.__init__(self, stream_id=stream_id, **neo_kwargs)
     38 
     39         #~ # open the corresponding stream probe

/media/luiz/storage/Github/spikeinterface/spikeinterface/extractors/neoextractors/neobaseextractor.py in __init__(self, stream_id, **neo_kwargs)
     24     def __init__(self, stream_id=None, **neo_kwargs):
     25 
---> 26         _NeoBaseExtractor.__init__(self, **neo_kwargs)
     27 
     28         # check channel

/media/luiz/storage/Github/spikeinterface/spikeinterface/extractors/neoextractors/neobaseextractor.py in __init__(self, **neo_kwargs)
     14         neoIOclass = eval('neo.rawio.' + self.NeoRawIOClass)
     15         self.neo_reader = neoIOclass(**neo_kwargs)
---> 16         self.neo_reader.parse_header()
     17 
     18         assert self.neo_reader.block_count() == 1, \

/media/luiz/storage/Github/python-neo/neo/rawio/baserawio.py in parse_header(self)
    183 
    184         """
--> 185         self._parse_header()
    186         self._check_stream_signal_channel_characteristics()
    187 

/media/luiz/storage/Github/python-neo/neo/rawio/spikeglxrawio.py in _parse_header(self)
     76 
     77     def _parse_header(self):
---> 78         self.signals_info_list = scan_files(self.dirname)
     79 
     80         # sort stream_name by higher sampling rate first

/media/luiz/storage/Github/python-neo/neo/rawio/spikeglxrawio.py in scan_files(dirname)
    257                 # except for the last fake channel
    258                 per_channel_gain = np.ones(num_chan, dtype='float64')
--> 259                 if meta['imDatPrb_type'] == '0':
    260                     if signal_kind == 'ap':
    261                         index_imroTbl = 3

KeyError: 'imDatPrb_type'

indeed, imDatPrb_type does not exist in meta file

samuelgarcia · 2022-01-07T12:32:25Z

which version of NP do you have ?
Can you send me the meta file ?

luiztauffer · 2022-01-07T13:12:17Z

It is caused by an older version of SpieGLX metadata, which does not have imDatPrb_type

This can be solved by this small change:

if 'imDatPrb_type' not in meta or meta['imDatPrb_type'] == '0':
    if signal_kind == 'ap':
        index_imroTbl = 3
    elif signal_kind == 'lf':
        index_imroTbl = 4
    for c in range(num_chan - 1):
        v = meta['imroTbl'][c].split(' ')[index_imroTbl]
        per_channel_gain[c] = 1. / float(v)

ref: https://github.com/billkarsh/SpikeGLX/blob/gh-pages/Support/Metadata_3A.md

The first entry of the Imec Readout Table (imRo) is a header. Here, (513180531,3,384) indicates probe serial number, probe option, number of channels. Each subsequent channel entry has five values:

Channel number,
Bank number of the connected electrode,
Reference ID index,
AP band gain,
LF band gain.

luiztauffer · 2022-01-07T13:29:07Z

neo/rawio/spikeglxrawio.py

-                # the last channel doesn't have a gain value
-                for c in range(num_chan - 1):
-                    per_channel_gain[c] = 1. / float(meta['imroTbl'][c].split(' ')[index_imroTbl])
+                if meta['imDatPrb_type'] == '0':


Suggested change

if meta['imDatPrb_type'] == '0':

if 'imDatPrb_type' not in meta or meta['imDatPrb_type'] == '0':

I am having the same problem

samuelgarcia · 2022-01-18T17:04:38Z

@luiztauffer @bendichter : I made a new commit that should fix you problem.
I read the metadata doc again. It should work for you.
About the neuropixel2.0 the gain was buggy because the gain factor was different.

@AntonioST can you test this on your neuropixel data and check the gain with the latest commit, please I think there was a bug ?

@JuliaSprenger : read to review and and merge I think now. We stringly need this in spikeinterface.

JuliaSprenger · 2022-01-18T19:56:32Z

which version of NP do you have ? Can you send me the meta file ?

Do you add this to the test datasets?

samuelgarcia · 2022-01-18T20:02:26Z

@JuliaSprenger : we already have in spikeglx NP1 and NP2 in the gin.
The one from Luiz is not in dataset but it behave like NP1 with very small change in metafile.
I think it is ok.

JuliaSprenger · 2022-01-18T20:22:01Z

Do you want to comment / resolve this one? https://github.com/NeuralEnsemble/python-neo/pull/1045/files#r746731986

JuliaSprenger · 2022-01-18T20:26:19Z

No I don't think we should add probeinterface as a dependency for neo. weak dep would be ok in that case.

To add the location is super easy
# read signals
glx_folder = '/bla/bla'
reader = neo.SpikeGLXIO(dirname=glx_folder)
anasig = reader.read_segment().analogsignals[0]
# read probe
probe = probeinterface.read_spikeglx(glx_folder / 'blabla.meta')
# set location in annotations
anasig.array_annotations['channel_location'] = probe.contact_positions
This is why I wanted to remove location handling from neo to avoid maintenance because it is already maintain in PI. Add weakref and doing theses lines internally is also possible. Making user dealing directly with probeinterface is also good...

I don't think users should need to interact with probeinterface if they don't have to. Can you either add this here (with weak ref) or document it somehow so it's visible to users. We could also make the channel location handling optional.

samuelgarcia · 2022-01-19T17:30:44Z

@JuliaSprenger : I added back channel_location in neo directly using a dep on probeinterface for this IO as you suggested.
Honestly I don't think it is very usefull to handle channel location here in a bad way (dim per dim) because we don't handle 2D array annotations.
I think using probeinterface directly should be a better practice.

neo/rawio/spikeglxrawio.py

JuliaSprenger · 2022-01-19T19:33:30Z

@samuelgarcia Do you want to wait some more for the feedback by @AntonioST and @luiztauffer or can I merge this now?

samuelgarcia · 2022-01-19T19:35:21Z

You can merge now.

samuelgarcia added 3 commits October 29, 2021 12:38

Handle channel gain for NP2.0 in SpikeGLXRawio

322029b

remove channel location because it was wrong. This is done correctly …

9114fae

…in probeinterface.

add load_sync_channel to not expose the last channel (SY0)

c2a90b5

Make test on new files and fix bugs.

dc9027a

Some clean

653b206

samuelgarcia marked this pull request as ready for review November 10, 2021 12:43

samuelgarcia mentioned this pull request Nov 10, 2021

Set probe for spikeglx with neo 0.11 (actual master) SpikeInterface/spikeinterface#353

Merged

some bug fix for spikeinterface case

3255ef2

JuliaSprenger reviewed Nov 10, 2021

View reviewed changes

fix gain bug due to bad ident

dac2916

JuliaSprenger mentioned this pull request Dec 24, 2021

[spikeglx] explicitly specify test dataset - fix master #1058

Closed

JuliaSprenger reviewed Dec 24, 2021

View reviewed changes

neo/test/rawiotest/test_spikeglxrawio.py Show resolved Hide resolved

luiztauffer reviewed Jan 7, 2022

View reviewed changes

Fix some gain for NP 2.0 and take in account for metadata version.

3095a57

codestyle

b2e6718

samuelgarcia added 2 commits January 19, 2022 14:19

Add back channel locations with probeinterface dependency.

f3d8424

Propagate to SpikeGLXIO

7b712ef

JuliaSprenger reviewed Jan 19, 2022

View reviewed changes

neo/rawio/spikeglxrawio.py Outdated Show resolved Hide resolved

oups

ff6e90e

JuliaSprenger reviewed Jan 19, 2022

View reviewed changes

neo/rawio/spikeglxrawio.py Show resolved Hide resolved

JuliaSprenger merged commit 2d0ba52 into NeuralEnsemble:master Jan 20, 2022

JuliaSprenger mentioned this pull request Mar 3, 2022

Neuropixels 2.0 support #1028

Closed

	if meta['imDatPrb_type'] == '0':
	if 'imDatPrb_type' not in meta or meta['imDatPrb_type'] == '0':

Spikeglx np2 #1045

Spikeglx np2 #1045

Uh oh!

Conversation

samuelgarcia commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pep8speaks commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2022-01-19 17:30:03 UTC

Uh oh!

samuelgarcia commented Nov 10, 2021

Uh oh!

samuelgarcia commented Nov 10, 2021

Uh oh!

JuliaSprenger commented Nov 10, 2021

Uh oh!

samuelgarcia commented Nov 10, 2021

Uh oh!

JuliaSprenger Nov 10, 2021

Choose a reason for hiding this comment

Uh oh!

JuliaSprenger Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JuliaSprenger commented Nov 10, 2021

Uh oh!

JuliaSprenger commented Nov 10, 2021

Uh oh!

samuelgarcia commented Nov 10, 2021

Uh oh!

samuelgarcia commented Nov 15, 2021

Uh oh!

JuliaSprenger commented Nov 16, 2021

Uh oh!

samuelgarcia commented Nov 16, 2021

Uh oh!

Uh oh!

luiztauffer commented Jan 7, 2022

Uh oh!

samuelgarcia commented Jan 7, 2022

Uh oh!

luiztauffer commented Jan 7, 2022

Uh oh!

luiztauffer Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bendichter Jan 14, 2022

Choose a reason for hiding this comment

Uh oh!

samuelgarcia commented Jan 18, 2022

Uh oh!

JuliaSprenger commented Jan 18, 2022

Uh oh!

samuelgarcia commented Jan 18, 2022

Uh oh!

JuliaSprenger commented Jan 18, 2022

Uh oh!

JuliaSprenger commented Jan 18, 2022

Uh oh!

samuelgarcia commented Jan 19, 2022

Uh oh!

Uh oh!

Uh oh!

JuliaSprenger commented Jan 19, 2022

Uh oh!

samuelgarcia commented Jan 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

samuelgarcia commented Oct 29, 2021 •

edited

Loading

pep8speaks commented Oct 29, 2021 •

edited

Loading

luiztauffer Jan 7, 2022 •

edited

Loading