Skip to content

White matter Recording Extractor Follow Up#3880

Merged
pauladkisson merged 3 commits intoSpikeInterface:mainfrom
catalystneuro:white_matter
Apr 22, 2025
Merged

White matter Recording Extractor Follow Up#3880
pauladkisson merged 3 commits intoSpikeInterface:mainfrom
catalystneuro:white_matter

Conversation

@pauladkisson
Copy link
Copy Markdown
Collaborator

@pauladkisson pauladkisson commented Apr 22, 2025

Follow up on #3849

I forgot to specify self._kwargs, which gave me trouble when trying to set the probe from probeinterface.

Now, self._kwargs is set properly and I added a test to make sure it doesn't get missed again.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes an issue with the WhiteMatterRecordingExtractor by ensuring that self._kwargs is properly set and adds a regression test.

  • Fix missing self._kwargs initialization in the extractor.
  • Add a new test (test_kwargs) to verify that _kwargs is propagated correctly.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/spikeinterface/extractors/whitematterrecordingextractor.py Updates init to initialize self._kwargs with the correct parameters.
src/spikeinterface/extractors/tests/test_whitematterrecordingextractor.py Introduces test_kwargs to validate consistency of initialization via _kwargs.
Comments suppressed due to low confidence (1)

src/spikeinterface/extractors/tests/test_whitematterrecordingextractor.py:62

  • [nitpick] Consider adding test cases for when channel_ids is None to ensure that the extractor correctly defaults channel_ids if not provided.
def test_kwargs():

assert recording.get_duration() == 1.0


def test_kwargs():
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see, yes, that's our bad as reviewers:

Instead of adding tests for kwargs specifically can you add a pickling/serialization tests like this:

https://github.com/h-mayorquin/spikeinterface/blob/786541e3c89abcd0d9f4be0ca2478259da3cba52/src/spikeinterface/extractors/tests/common_tests.py#L90-L102

That should cover the kwargs.

We should add some base testing that is for all recordings that contains this too but this is for ourselves in another PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding tests for kwargs specifically can you add a pickling/serialization tests like this:

Done!

@h-mayorquin
Copy link
Copy Markdown
Collaborator

What was the error you were getting when setting the probe, btw?

@pauladkisson pauladkisson merged commit 3302570 into SpikeInterface:main Apr 22, 2025
15 checks passed
@pauladkisson pauladkisson deleted the white_matter branch April 22, 2025 21:19
@pauladkisson
Copy link
Copy Markdown
Collaborator Author

What was the error you were getting when setting the probe, btw?

Traceback (most recent call last):
  File "/Users/pauladkisson/Documents/CatalystNeuro/SchneiderConv/schneider-lab-to-nwb/src/schneider_lab_to_nwb/corredera_2025/corredera_2025_convert_session.py", line 151, in <module>
    main()
    ~~~~^^
  File "/Users/pauladkisson/Documents/CatalystNeuro/SchneiderConv/schneider-lab-to-nwb/src/schneider_lab_to_nwb/corredera_2025/corredera_2025_convert_session.py", line 137, in main
    session_to_nwb(
    ~~~~~~~~~~~~~~^
        ephys_file_path=ephys_file_path,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ...<7 lines>...
        verbose=verbose,
        ^^^^^^^^^^^^^^^^
    )
    ^
  File "/Users/pauladkisson/Documents/CatalystNeuro/SchneiderConv/schneider-lab-to-nwb/src/schneider_lab_to_nwb/corredera_2025/corredera_2025_convert_session.py", line 116, in session_to_nwb
    converter.run_conversion(metadata=metadata, nwbfile_path=nwbfile_path, conversion_options=conversion_options)
    ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/pauladkisson/Documents/CatalystNeuro/Neuroconv/neuroconv/src/neuroconv/nwbconverter.py", line 322, in run_conversion
    nwbfile = self.create_nwbfile(metadata=metadata, conversion_options=conversion_options)
  File "/Users/pauladkisson/Documents/CatalystNeuro/Neuroconv/neuroconv/src/neuroconv/nwbconverter.py", line 229, in create_nwbfile
    self.add_to_nwbfile(nwbfile=nwbfile, metadata=metadata, conversion_options=conversion_options)
    ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/pauladkisson/Documents/CatalystNeuro/Neuroconv/neuroconv/src/neuroconv/nwbconverter.py", line 249, in add_to_nwbfile
    data_interface.add_to_nwbfile(
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
        nwbfile=nwbfile, metadata=metadata, **conversion_options.get(interface_name, dict())
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "/Users/pauladkisson/Documents/CatalystNeuro/SchneiderConv/schneider-lab-to-nwb/src/schneider_lab_to_nwb/corredera_2025/corredera_2025_white_matter_recording_interface.py", line 37, in add_to_nwbfile
    self.recording_extractor = self.recording_extractor.set_probe(probe, group_mode="by_shank")
                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/pauladkisson/Documents/CatalystNeuro/Neuroconv/catalystneuro/spikeinterface/src/spikeinterface/core/baserecordingsnippets.py", line 98, in set_probe
    return self._set_probes(probegroup, group_mode=group_mode, in_place=in_place)
           ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/pauladkisson/Documents/CatalystNeuro/Neuroconv/catalystneuro/spikeinterface/src/spikeinterface/core/baserecordingsnippets.py", line 192, in _set_probes
    sub_recording = self.clone()
  File "/Users/pauladkisson/Documents/CatalystNeuro/Neuroconv/catalystneuro/spikeinterface/src/spikeinterface/core/base.py", line 607, in clone
    clone = BaseExtractor.from_dict(d)
  File "/Users/pauladkisson/Documents/CatalystNeuro/Neuroconv/catalystneuro/spikeinterface/src/spikeinterface/core/base.py", line 567, in from_dict
    extractor = _load_extractor_from_dict(dictionary)
  File "/Users/pauladkisson/Documents/CatalystNeuro/Neuroconv/catalystneuro/spikeinterface/src/spikeinterface/core/base.py", line 1123, in _load_extractor_from_dict
    extractor = extractor_class(**new_kwargs)
TypeError: WhiteMatterRecordingExtractor.__init__() got an unexpected keyword argument 'file_paths'. Did you mean 'file_path'?

@h-mayorquin
Copy link
Copy Markdown
Collaborator

Ah, it was because set_probe copies the recording and that implies serialization which needs kwargs. 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.

3 participants