WhiteMatterRecordingExtractor #3849

pauladkisson · 2025-04-08T19:42:18Z

Pretty minimal wrapper around BinaryRecordingExtractor with a specified file_offset, dtype, and gain. As far as I know these should be constant for WhiteMatter files, but I only have one example, and no formal spec from WhiteMatter (so far). So, I think we should keep an eye out for WhiteMatter files in future conversions and be prepared to update this extractor as necessary.

alejoe91

Thanks @pauladkisson

See comments

alejoe91 · 2025-04-10T09:43:24Z

src/spikeinterface/extractors/whitematterrecordingextractor.py

+    @staticmethod
+    def write_recording(recording, file_paths, **job_kwargs):
+        """
+        Save the traces of a recording extractor in binary .dat format.
+
+        Parameters
+        ----------
+        recording : RecordingExtractor
+            The recording extractor object to be saved in .dat format
+        file_paths : str
+            The path to the file.
+        """
+        job_kwargs["byte_offset"] = 8
+        BinaryRecordingExtractor.write_recording(recording, file_paths=file_paths, dtype="int16", **job_kwargs)


I would remove this and just write it in the test

src/spikeinterface/extractors/whitematterrecordingextractor.py

src/spikeinterface/core/tests/test_whitematterrecordingextractor.py

src/spikeinterface/extractors/whitematterrecordingextractor.py

h-mayorquin · 2025-04-10T14:49:24Z

src/spikeinterface/extractors/whitematterrecordingextractor.py

+    @staticmethod
+    def write_recording(recording, file_paths, **job_kwargs):
+        """
+        Save the traces of a recording extractor in binary .dat format.
+
+        Parameters
+        ----------
+        recording : RecordingExtractor
+            The recording extractor object to be saved in .dat format
+        file_paths : str
+            The path to the file.
+        """
+        job_kwargs["byte_offset"] = 8
+        BinaryRecordingExtractor.write_recording(recording, file_paths=file_paths, dtype="int16", **job_kwargs)


h-mayorquin

This LGTM.
I think the read small traces test should be removed. No need to test here was is tested already.

I would like to have testing data on gin for this but I think @alejoe91 should make the call if this is necessary or not for this for merging.

Otherwise this has my +1.

src/spikeinterface/core/tests/test_whitematterrecordingextractor.py

h-mayorquin · 2025-04-10T17:20:06Z

src/spikeinterface/core/tests/test_whitematterrecordingextractor.py

+from spikeinterface.core.numpyextractors import NumpyRecording
+
+
+def test_round_trip(tmp_path):


This is OK but I would prefer testing data. Can we add something to gin? This should be easy to stub.

Ok, this is awaiting gin access. Should be up in a day or two.

See https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/155

GIN PR is merged :)

Ok, waiting for the test and should be good to go after that.

Ok, waiting for the test and should be good to go after that.

Done!

zm711 · 2025-04-11T12:24:36Z

Two minor comments unrelated to the PR content.

We typically do a forking workflow. Our docs do say that for dev, but lately we've been sneaking in more PRs from project branches. Do we want to loosen that requirement for people affiliated with the project or do we want to be strict about this (other than CI/release branches)
Is there a reason why we wouldn't want to implement this at the Neo level just to keep things separate? is it a concern about Neo releases? If so we could do the extractor like this for now, but work on a neo level and then convert? I think there is a benefit to keep as much non-sorting extractor stuff hived off. But if there is a good reason for us to not do that I would love to know people's opininons.

pauladkisson · 2025-04-11T16:02:53Z

We typically do a forking workflow. Our docs do say that for dev, but lately we've been sneaking in more PRs from project branches. Do we want to loosen that requirement for people affiliated with the project or do we want to be strict about this (other than CI/release branches)

I'm sorry I didn't follow the recommended forking workflow. I just defaulted to the branching workflow that we use with neuroconv, but I can be more diligent about it in the future.

Is there a reason why we wouldn't want to implement this at the Neo level just to keep things separate? is it a concern about Neo releases? If so we could do the extractor like this for now, but work on a neo level and then convert? I think there is a benefit to keep as much non-sorting extractor stuff hived off. But if there is a good reason for us to not do that I would love to know people's opininons.

@h-mayorquin and I decided against including this in neo since there is no header to parse, so it seemed like it wouldn't naturally lend itself to the neo structure. Also worth noting that BinaryRecordingExtractor (the parent of this class) does not depend on neo.

alejoe91 · 2025-04-11T16:39:29Z

I agree it's not worth including in NEO at this point!

zm711

Couple comments :)

src/spikeinterface/extractors/whitematterrecordingextractor.py

zm711 · 2025-04-11T17:03:51Z

src/spikeinterface/extractors/whitematterrecordingextractor.py

@@ -0,0 +1,62 @@
+from pathlib import Path
+from typing import List, Union, Optional


We are far enough along in python that we can use the built-in list, but since you're pulling in Union and Optional if you want to use List it doesn't cost extra I suppose.

src/spikeinterface/extractors/whitematterrecordingextractor.py

Co-authored-by: Zach McKenzie <[email protected]>

pauladkisson · 2025-04-15T15:11:37Z

Any other issues holding up this PR?

h-mayorquin · 2025-04-15T15:59:57Z

Data and removing of unnecessary test for me. Either Alessio and Sam should take a look as well.

pauladkisson · 2025-04-18T20:05:18Z

Not sure why the file is not being found...

zm711 · 2025-04-18T20:33:49Z

Not sure why the file is not being found...

I think our tests do not update the ephy_cache all the time. So I'm wondering if your specific file changes aren't triggering a cache check and so you're not getting the updated version. GIN is down this weekend for maintenance, but if this still isn't working Monday after a cache refresh from the cron job then I think we should explore this more.

h-mayorquin

Let's make the test work and this should be ready to go.

src/spikeinterface/core/tests/test_whitematterrecordingextractor.py

h-mayorquin · 2025-04-21T14:47:36Z

src/spikeinterface/core/tests/test_whitematterrecordingextractor.py

+
+
+file_path = (
+    get_global_dataset_folder()


This is not meant to work this way.

You need to use download_dataset:

https://github.com/h-mayorquin/spikeinterface/blob/f73c3e6ccb48dd113b6a9348b59245adc26ec106/src/spikeinterface/core/datasets.py#L12-L17

See an use here in the common test suite:

https://github.com/h-mayorquin/spikeinterface/blob/f73c3e6ccb48dd113b6a9348b59245adc26ec106/src/spikeinterface/extractors/tests/common_tests.py#L16-L25

And we should check that the data gets in the cache.

pauladkisson · 2025-04-21T16:00:53Z

Tests are passing now! (minus linux which should be fixed by #3877)

h-mayorquin

I am OK with this.

pauladkisson added 5 commits April 8, 2025 12:41

added WhiteMatterRecordingExtractor

b765552

cleaned up extractor

c49d55b

added tests

7f3e17f

removed unused imports

c4b612d

Merge branch 'main' into white_matter

2d7c1b8

pauladkisson marked this pull request as ready for review April 9, 2025 19:09

pauladkisson requested a review from alejoe91 April 9, 2025 19:09

pauladkisson mentioned this pull request Apr 9, 2025

WhiteMatter catalystneuro/neuroconv#1297

Merged

alejoe91 requested changes Apr 10, 2025

View reviewed changes

alejoe91 added the extractors Related to extractors module label Apr 10, 2025

h-mayorquin reviewed Apr 10, 2025

View reviewed changes

pauladkisson added 5 commits April 11, 2025 02:34

Merge branch 'main' into white_matter

e53b327

switched to file_path instead of file_paths

b45b063

removed write_recording

2cf25b9

removed unnecessary

12e5c30

added read_whitematter to api

763d7bc

pauladkisson requested review from alejoe91 and h-mayorquin April 10, 2025 16:55

h-mayorquin reviewed Apr 10, 2025

View reviewed changes

Merge branch 'main' into white_matter

cfe2ad0

zm711 reviewed Apr 11, 2025

View reviewed changes

pauladkisson and others added 2 commits April 12, 2025 04:19

Update src/spikeinterface/extractors/whitematterrecordingextractor.py

861eb8b

Co-authored-by: Zach McKenzie <[email protected]>

updated wm params

3b3bde1

pauladkisson mentioned this pull request Apr 11, 2025

Schneider Lab Conversion Progress catalystneuro/schneider-lab-to-nwb#1

Open

38 tasks

Merge branch 'main' into white_matter

9f70076

pauladkisson and others added 5 commits April 15, 2025 11:05

removed sequential reading test

8d6cbf1

moved wm params to class variable

1b1233e

Merge branch 'main' into white_matter

5937ab8

Merge branch 'main' into white_matter

366ff1d

added test_on_data

7fcec58

pauladkisson requested a review from h-mayorquin April 18, 2025 19:55

moved file_path outside test

53397f6

h-mayorquin requested changes Apr 21, 2025

View reviewed changes

pauladkisson added 2 commits April 21, 2025 08:46

download_dataset

5a80841

moved whitematter test to extractors

5eb1417

h-mayorquin approved these changes Apr 21, 2025

View reviewed changes

Merge branch 'main' into white_matter

e7ed9ab

alejoe91 approved these changes Apr 22, 2025

View reviewed changes

alejoe91 merged commit 28cb527 into main Apr 22, 2025
15 checks passed

h-mayorquin deleted the white_matter branch April 22, 2025 13:12

pauladkisson mentioned this pull request Apr 22, 2025

White matter Recording Extractor Follow Up #3880

Merged

		from spikeinterface.core.numpyextractors import NumpyRecording


		def test_round_trip(tmp_path):

		@@ -0,0 +1,62 @@
		from pathlib import Path
		from typing import List, Union, Optional

WhiteMatterRecordingExtractor #3849

WhiteMatterRecordingExtractor #3849

Uh oh!

Conversation

pauladkisson commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alejoe91 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

h-mayorquin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zm711 commented Apr 11, 2025

Uh oh!

pauladkisson commented Apr 11, 2025

Uh oh!

alejoe91 commented Apr 11, 2025

Uh oh!

zm711 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pauladkisson commented Apr 15, 2025

Uh oh!

h-mayorquin commented Apr 15, 2025

Uh oh!

pauladkisson commented Apr 18, 2025

Uh oh!

zm711 commented Apr 18, 2025

Uh oh!

h-mayorquin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pauladkisson commented Apr 21, 2025

Uh oh!

h-mayorquin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pauladkisson commented Apr 8, 2025 •

edited

Loading