SpikeInterface / MEArec

Fast and customizable of extracellular recordings on MEA
GNU General Public License v3.0
42 stars 19 forks source link

filtering order bug & enhancement advice #142

Closed ZoeChen96 closed 1 year ago

ZoeChen96 commented 1 year ago

Hi there,

I am generating MEArec datasets with different filtering options. I found two issues: 1) In MEArec filtering, I saw you use the filtfilt function. But if we want to mimic the analog filter applied to the signal, I think we need a causal filter (because in analog system we don't have the option to filter back in time), which means using functions such as lfilter. For more information, you can refer to the following paper , part 2.2:

Navajas, Joaquin, et al. "Minimum requirements for accurate and efficient real-time on-chip spike sorting." Journal of neuroscience methods 230 (2014): 51-64.

Or at least, shall we have the option to choose between filtfilt and filter?

2) filtering order is not working. I configure the filter as following:

recording_params['recordings']['filter'] = True
recording_params['recordings']['filter_cutoff'] = [300, 14999] 
recording_params['recordings']['filter_order'] = 2

Then I found the filter cutoff works, but the filter order does not. (By checking the waveforms). I go inside the code, and found the parameter order does assigned to args in https://github.com/alejoe91/MEArec/blob/aa3d00ad6c4fea4e3f5fbd9401159e92e262dc07/MEArec/generators/recordinggenerator.py#L1276, but in the file tools.py, it seems to be overwrite by the default value 3: https://github.com/alejoe91/MEArec/blob/aa3d00ad6c4fea4e3f5fbd9401159e92e262dc07/MEArec/tools.py#LL2657C5-L2657C26. I tried to delete the default parameter 3, but it gives me an error of

Traceback (most recent call last):
  File "/imec/other/macaw/projectdata/mearec_datasets/short_datasets/recording_gen.py", line 1, in <module>
    import MEArec as mr
  File "/imec/other/macaw/chen14/gitrepository/MEArec/MEArec/__init__.py", line 1, in <module>
    from MEArec.generation_tools import (gen_recordings, gen_spiketrains,
  File "/imec/other/macaw/chen14/gitrepository/MEArec/MEArec/generation_tools.py", line 10, in <module>
    from .generators import (RecordingGenerator, SpikeTrainGenerator,
  File "/imec/other/macaw/chen14/gitrepository/MEArec/MEArec/generators/__init__.py", line 1, in <module>
    from .recordinggenerator import RecordingGenerator
  File "/imec/other/macaw/chen14/gitrepository/MEArec/MEArec/generators/recordinggenerator.py", line 21, in <module>
    from ..tools import (annotate_overlapping_spikes, compute_modulation,
  File "/imec/other/macaw/chen14/gitrepository/MEArec/MEArec/tools.py", line 2657
    def filter_analog_signals(signals, freq, fs, filter_type='bandpass', order):
                                                                         ^^^^^
SyntaxError: non-default argument follows default argument

Could you please have a look?

alejoe91 commented 1 year ago

Hi @ZoeChen96

  1. I agree we can add another parameter called filter_type="filtfilt" | "lfilter"

  2. The function is defined correctly. The order=3 is just the default value, but it should be overwritten when you pass it externally.

After checking, I indeed noticed that the order is not well propagated to the bandpass case! Making a PR to fix both issues

ZoeChen96 commented 1 year ago

Thank you very much!