add set_probe method to BaseRecordingExtractorInterface

[magland]

@alejoe91 @CodyCBakerPhD as we were discussing in #634

[CodyCBakerPhD]

Just need some unit tests for this; would you be willing to add a general tutorial in the user guide section as well in this PR or would you prefer that in a follow-up?

Basically, what a user would need is a clear way to 'how' to get that probe object. I know SI/PI has a couple of ways (including some predefined that can be loaded? But they can also be set entirely in-memory, as well as that CSV approach you used)

[magland]

Just need some unit tests for this; would you be willing to add a general tutorial in the user guide section as well in this PR or would you prefer that in a follow-up?

Basically, what a user would need is a clear way to 'how' to get that probe object. I know SI/PI has a couple of ways (including some predefined that can be loaded? But they can also be set entirely in-memory, as well as that CSV approach you used)

Yeah I think we can include the tutorial in this PR. Will probably need help from @alejoe91

I know Alessio also wanted to add a set_probe_group

[magland]

Is there a way to get the test data on my local machine so I can run the tests that require access to that data?

[CodyCBakerPhD]

Is there a way to get the test data on my local machine so I can run the tests that require access to that data?

Yep, just give our Developer Guide a perusal

I would say though, that some cleaner unit tests might use the mock class (which uses randomly generated data via SI) https://github.com/catalystneuro/neuroconv/blob/main/src/neuroconv/tools/testing/mock_interfaces.py#L121 and belong in approximately this location of the testing suite: https://github.com/catalystneuro/neuroconv/blob/main/tests/test_ecephys/test_ecephys_interfaces.py

The tutorial could (probably should) use some of the real testing data however; but the only testing dataset I actually know the probe of would be https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/src/master/neuroscope/dataset_1 which became dandiset 000003 and has probe models defined in the paper

[magland]

@CodyCBakerPhD
This is a bit sloppy but it's the best I can do for now...

I added a call to interface.set_probe() in the tests

https://github.com/magland/neuroconv/blob/b8f793b5b7737d9c6789618bd80147a590b9f4d3/src/neuroconv/tools/testing/data_interface_mixins.py#L104-L130

This is only for the Intan interface, because when I did this for all interfaces, then I get into trouble with other tests that expect that the number of groups is 1.

And then I make sure the nwb recording matches with the properties rel_x, rel_y, rel_z, and group:

https://github.com/magland/neuroconv/blob/b8f793b5b7737d9c6789618bd80147a590b9f4d3/src/neuroconv/tools/testing/data_interface_mixins.py#L333-L342

[CodyCBakerPhD]

@magland Thanks for working on this! The core function is nice and simple, but we should really make sure it can be used without error on as many formats as possible notwithstanding highly specific issues to be propagated up to SI/neo

[alejoe91]

just one note: if the probe has less channels than the recording (e.g., you want to only select the A channels from an Intan recording), the set_probe(..., in_place=True) will fail.

We can't do anything about it, just something to keep in mind :)

[CodyCBakerPhD]

@alejoe91 What would this represent in a real world case?

Would it be indicative of a bad probe definition by the user?

Or are there cases where the user might actually specify a probe correctly, but extra channels show up (possibly from auxiliary sources or a mistake in SI/neo)?

[alejoe91]

yes I think the second option is what I had in mind. You have a recording with Ephys + Aux channels and you might want to select ephys channels with the probe slicing

[magland]

@magland Thanks for working on this! The core function is nice and simple, but we should really make sure it can be used without error on as many formats as possible notwithstanding highly specific issues to be propagated up to SI/neo

Thanks @CodyCBakerPhD

I think I've addressed all of your comments, except possibly the location of the generate_mock_probe(). In particular, the test is now applied to all recording extractor interfaces.

[CodyCBakerPhD]

I think I've addressed all of your comments

Thanks, looking a lot better!

except possibly the location of the generate_mock_probe().

That place is fine, we can move later if we want

In particular, the test is now applied to all recording extractor interfaces.

Great!

One last comment about where the probe injection occurs in the Mixin class

[magland]

@CodyCBakerPhD I have moved the set_probe() test code to the RecordingExtractorInterfaceTestMixin class, and I have used the not has_probe() condition for setting the probe.

[codecov]

Codecov Report

Merging #639 (5b3a999) into main (df5a1a4) will increase coverage by 0.07%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #639      +/-   ##
==========================================
+ Coverage   91.65%   91.72%   +0.07%     
==========================================
  Files         107      108       +1     
  Lines        5653     5703      +50     
==========================================
+ Hits         5181     5231      +50     
  Misses        472      472              

Flag	Coverage Δ
unittests	91.72% <100.00%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...erfaces/ecephys/baserecordingextractorinterface.py	97.00% <100.00%> (+0.15%)	⬆️
...c/neuroconv/tools/testing/data_interface_mixins.py	95.64% <100.00%> (+0.24%)	⬆️
src/neuroconv/tools/testing/mock_probes.py	100.00% <100.00%> (ø)

[CodyCBakerPhD]

Looks great to me! Thanks for all the hard work on this