spikeinterface icon indicating copy to clipboard operation
spikeinterface copied to clipboard

Recorders with non voltage (non standard) channels and slicing channels

Open h-mayorquin opened this issue 2 years ago • 10 comments

In #761 we move the warnings of non standard units to get_traces. We achieved this by introducing an extra attribute:

https://github.com/SpikeInterface/spikeinterface/blob/486bc67d364745437ebf1ed90bfdef7fae255c1c/spikeinterface/core/baserecording.py#L109-L116

However, as far as I know this attribute (self.has_non_standards_units) is not copied when we slice the channels with recorder.slice_channel which indicates problems. I think that what would happen right now is that the attribute NeoRawIOClass is not copied either so the sliced recorder with non voltage units will never raise the warning.

Maybe we should handle this with annotations as you already have the machinery in those methods to copy that?

Context: #758, #761 and https://github.com/NeuralEnsemble/python-neo/issues/1133

h-mayorquin avatar Jul 04 '22 16:07 h-mayorquin

@h-mayorquin you can extend the copy_metadata function: https://github.com/SpikeInterface/spikeinterface/blob/master/spikeinterface/core/base.py#L227

alejoe91 avatar Jul 04 '22 17:07 alejoe91

I think I would go for an annotation like this '__has_non_standards_units__' and not an attribute.

samuelgarcia avatar Jul 05 '22 10:07 samuelgarcia

@alejoe91 we should probably move this warning to get_traces to the neobase recording withing the library.

h-mayorquin avatar Jul 05 '22 14:07 h-mayorquin

what if you slice ? I think in base it is OK.

samuelgarcia avatar Jul 05 '22 15:07 samuelgarcia

Hi suggest adding this to the NeoBaseRecordingExtractor:

def get_traces(self,
               segment_index: Union[int, None] = None,
               start_frame: Union[int, None] = None,
               end_frame: Union[int, None] = None,
               channel_ids: Union[Iterable, None] = None,
               order: Union[str, None] = None,
               return_scaled=False,
               cast_unsigned=False
               ):
    if return_scaled:
            if hasattr(self, "NeoRawIOClass"):
                if self.has_non_standard_units:
                    message = ( 
                    f'This extractor based on neo.{self.NeoRawIOClass} has channels with units not in (V, mV, uV)'
                    )
                    warnings.warn(message)
    return super().get_traces(segment_index, start_frame, end_frame, channek_ids, order, return_scaled, cast_unsigned)

This would have the same effect without "polluting" the base with NEO related stuff (the BaseRecording doesn't even know neo exists!!!)

alejoe91 avatar Jul 19 '22 08:07 alejoe91

ok for me.

samuelgarcia avatar Jul 19 '22 08:07 samuelgarcia

except the cascade of if that could be done in one line with and

samuelgarcia avatar Jul 19 '22 08:07 samuelgarcia

except the cascade of if that could be done in one line with and

????

alejoe91 avatar Jul 19 '22 08:07 alejoe91

All right, so we should:

  1. Do this in NeoBaseRecordingExtractor as @alejoe91 points out, and
  2. Do this with attributes so the copying machinery of slicing works.

Please say if you agree with both and I will take care of it,

h-mayorquin avatar Jul 19 '22 09:07 h-mayorquin

@h-mayorquin please go ahead!

alejoe91 avatar Aug 02 '22 08:08 alejoe91