Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove lack of units in NeoBaseExtractors #758

Merged

Conversation

h-mayorquin
Copy link
Collaborator

This output to standard output is causing problems in some of our doctests in:

catalystneuro/nwb-conversion-tools#573

I think that this code serves not purpose. The user of a recording interface does not have a way to change the units. I think this information is useful for developers but as it stands I think it could be replaced by appropriate notes in the corresponding extractor or back in neo.

What do you think, @samuelgarcia?

To describe the problem in more detail look at the following example:

from pathlib import Path
from spikeinterface.extractors import SpikeGadgetsRecordingExtractor
import spikeextractors as se

DATA_PATH = Path("/home/heberto/ephy_testing_data/")
file_name = "W122_06_09_2019_1_fromSD"
file_path=DATA_PATH / "spikegadgets" / f"{file_name}.rec"

recorder = SpikeGadgetsRecordingExtractor(file_path=file_path, stream_id="trodes")

Output:

This extractor based on  neo.SpikeGadgetsRawIO have strange units not in (V, mV, uV) ['' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' ''
 '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' ''
 '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' ''
 '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' ''
 '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' '' ''
 '' '' '' '' '' '' '' '']

@samuelgarcia
Copy link
Member

It is still a bit important from spiekinetrface point of view.

  • spikeinterface do not handle units. Every thing is supposed to be microVolt because extra cellular recording are micro volts
  • neo handle explicitly units of any kind (uV, mV, V, pA, ...) and also the fact that there is no units in a file (dimentionless or '')

Since we wrapper on top on neo in some case like the one you show we cannot garanty to the user that the trace can be scaled to micro volt when rec.get_traces(..., return_scaled=True) so we need a warning at some point. Here the warning is init.
It could be only we we do a get_traces() scaled.

@alejoe91 : what do you think ?

@h-mayorquin : maybe for spike gadget we can patch the neo SpikeGadgetsIO and check if we have information about the tru units maybe they are in micro volt maybe not we need specification from the vendor and so some discussion by mail.

@h-mayorquin
Copy link
Collaborator Author

I think that:

  1. Long term it should be patched where we can in Neo.
  2. Middle term this informantion should be provided when the user calls rec.get_traces(..., return_scaled=True)
  3. Short term, can we make this into a warning here? I will change it to a warning in the commit.

Also, I am surprised this change is generating a sorting error in the full spikesorter tests, any idea about that?

@samuelgarcia
Copy link
Member

Lets go for midterm directly, no ?

@h-mayorquin
Copy link
Collaborator Author

Lets go for midterm directly, no ?

I can do it in this PR.
Whata bout the error, @samuelgarcia ? Any ideas?

@h-mayorquin
Copy link
Collaborator Author

@samuelgarcia
I implemented the short term fix here. The middle term should be implemented in #761. Maybe we can merge this as I need it and then fine tune with the other PR.

@samuelgarcia samuelgarcia merged commit d26f991 into SpikeInterface:master Jun 30, 2022
@samuelgarcia
Copy link
Member

done . Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants