BlackrockRawIO FileSpec 3.0

[cboulay]

Closes #795
Blackrock_FileSpec_3_0.zip

[JuliaSprenger]

Hi @cboulay Thanks for adding support for the version 3.0. Do you mind if I add the files you provided to our set of test files on gin, so we can continually test these? In that case whom can I list as official contributor of the files? Also I noticed the corresponding ccf file is relatively large for not being actually used by the reader. Is there some redundant information in there that we could remove for the usage as test file?

[samuelgarcia]

Hi Chadwick, this is really really cool.

[cboulay]

@JuliaSprenger, yes you can use the files however you like. I recorded them but the input to the system comes from Blackrock's Digital Neural Signal Simulator, so it's not "real" data. You can list me as the official contributor, if only because I will probably be a good contact person for a while. As far as I'm concerned, it's in public domain. I'm sure Blackrock will have a file similar to this available for download from their website sooner or later.

I was pretty surprised at the size of the CCF too, especially given that I only recorded such a small number of channels. blackrockrawio does not use ccf files so you can delete the whole thing.

[JuliaSprenger]

I added the files to the gin repository (https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/86). As soon as that PR is merged it would be good to include the new dataset in the neo tests. Could you then add the corresponding reference in the RawIO tests here and in the IO tests here?

[JuliaSprenger]

The PR on gin is merged. You should now be able to add the files to the tests.

[cboulay]

Thanks @JuliaSprenger .

I don't love the idea of comparing to the Matlab loader. I actually don't have a Matlab license at the moment.

Would it be sufficient to test the shape, the type, the range of values, and other metrics like that?
If the values need to be tested directly then I can do that for 1-2 channels.

[JuliaSprenger]

Yes, I agree. Comparing the data loaded to an alternative way to access the same data is would be a very advanced test already. As a first step just verifying that the IO can load these files and that some data values are loaded is completely fine. If you are motivated to also add a test checking for the correctness of these values then storing some of these as plain text is also an option (as it's done e.g. in the NeuralynxIO tests).

[cboulay]

I got around to attempting to write the unit tests but I failed. It seems I can't run unit tests for a module unless I have the exact environment. I cannot install git-annex using the DataLad installer instructions because I get datalad_installer.MethodNotSupportedError: Darwin OS not supported.

I put the files in the correct location manually but then they were erased when I attempted to run a test. That seems intentional.
https://github.com/NeuralEnsemble/python-neo/blob/master/neo/utils/datasets.py#L64-L67

Is there another way?

[cboulay]

I spun up my Linux box and I was able to run the tests. I identified an issue. There seems to be a large number of segments in the .nev file but not in the .ns6 file. Currently this is manifesting as an error when testing spike count compliance, but that's a misdirection.

The real reason is that some events are written with newer timestamps than the spikes that are being written. I will open a new issue about that.

[cboulay]

Tests pending outcome of #1181

[cboulay]

375e474 fix for #1181 is necessary for the tests to pass if we want to use this same example file. If the proposed solution of ignoring all unhandled events is too broad then we can add the problematic event types to the list of handled events then explicitly ignore those problematic types only.

Even if Blackrock decides to stop storing the events that are creating the issue, there might still be some files in the wild that have this issue so we need some kind of solution.

[cboulay]

Hi @JuliaSprenger , I decided not to add anymore tests than just the common ones unless you have any specific requests. Please let me know if anything else is required before merging.

[JuliaSprenger]

375e474 fix for #1181 is necessary for the tests to pass if we want to use this same example file. If the proposed solution of ignoring all unhandled events is too broad then we can add the problematic event types to the list of handled events then explicitly ignore those problematic types only.

Thanks for adding the fix for this issue, it looks appropriate to me.

Even if Blackrock decides to stop storing the events that are creating the issue, there might still be some files in the wild that have this issue so we need some kind of solution.

Yes for backward compatibility we should ignore these event types in any case.

[JuliaSprenger]

Hi @JuliaSprenger , I decided not to add anymore tests than just the common ones unless you have any specific requests. Please let me know if anything else is required before merging.

The common tests are fine for now as these automatically cover the new lines you added

[cboulay]

I added another commit that doesn't hurt anything here but future-proofs against another change that will come in a future release. The file spec won't change but some assumptions about timestamp resolution will no longer be valid so hard-coded 30_000 will be incorrect in some places.

[cboulay]

Looks like my latest commit doesn't work with older file specs. Sorry about that. I'll back it out and create a better version in the future when this new feature is actually released.

[JuliaSprenger]

Tests are passing now. @cboulay Thanks for the work you put into this!