Add support for Blackrock NSx files with per-sample PTP nanosecond timestamps #1338
Conversation
|
Hello @cboulay! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-10-28 18:00:59 UTC |
|
OK this is ready for review. If you want a sample data file then that will have to wait until Friday when I'm in front of the hardware needed to produce the 3.0-ptp variant file spec. |
|
Hi Chadwick. |
|
I found and fixed a bug in my previous implementation thanks to unit tests I just added. Here's a zip of the test files. If you can provide me instructions on how to created a pull request on GIN then I can go ahead and do that, but I'm also totally satisfied having someone else do it if you're already setup for that. |
|
@cboulay, test files were merged, but now you have some merge conflicts 1) we've run black on the code base and 2) we switched over to f-strings rather than % or .format. Did you want to try to touch those up? |
|
OK I've rebased and fixed the conflicts along the way. Let's hope the tests pass. BTW, I don't mind if you squash the commits when merging... there was some back and forth on quotes that become obsolete when you adopted PEP8. |
|
Actually I went ahead and squashed into 1 commit. The unit test isn't running because it can't find the file. I guess I'm not providing its path correctly. Can anyone help me out with that part? |
There was a problem hiding this comment.
@cboulay see my comment and see if it looks reasonable.
See test structure here:
https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/src/master/blackrock/blackrock_3_0_ptp
… nanosecond timestamps blackrockrawio - fix backwards compatibility by retrieving 'timestamp_resolution' from another location for older file spec. blackrockrawio - fix calculation of clk threshold to identify segment breaks. blackrockrawio - add unit test for filespec 3.0 ptp
|
I'm pretty lost trying to figure out what's going on the with file loading. I'm happy to yield to someone else. |
|
We are planning on including this in the next release which will go out in the next few days so if you have the time to either implement my commit fix or give us access we will include otherwise we will plan on this for the next release. |
|
I don't see the option to allow edits from maintainers. I found the GH docs that explains it but the option isn't on this page. |
|
Normally there is a little check box on the right side of the page just below all the info. When I first looked at your PR you had it turned on but it got switched off at some point. Either way we will see if tests pass now! |
|
Seems like it failed. Could you doublecheck the gin link I gave you and make sure it has all the files necessary for blackrock? |
|
Ah, it's because I made the PR from the lab org. sigh |
|
So I think there are two paths that need to be fixed. One at the top and one at the bottom. Since I can't push stuff here do you want a PR on your branch with the two file path fixes I think we need and we can test? |
|
I tried copying the pattern of the other files (blackrock_2_1, blackrock_3_0). It works locally but not on the runner. Your suggested changes break from that pattern and don't work locally nor on the runner. I'll make a new PR from my user account then grant edit permissions. |
|
That's cool. One last check: could you try this as the filename? Previously you didn't have the -001 right? so maybe it needs the full file name for the .nev file? 20231027-125608-001 |
|
Perfect that worked! thanks for bearing with me! The dirname messed with me! We should really call that filename in the tests for this IO (not your fault, something we've been discussing #1455 recently). |
|
Seems to be working. Thanks for your help! |
|
@samuelgarcia tests are passing here, but you also said you would look over the code. Did you want to read this over first before we merge? EDIT: Just pinging @samuelgarcia again since I sent the first message later on Friday evening. |
|
Thanks a lot for this new add Chadwick. |
Fixes #1332
Blackrock's newest signal acquisition devices use nanosecond timestamps from clocks synchronized with PTP. This allows synchronization between devices that might have slightly different clock rates and not capture exactly 30_000 samples per reference second.
Individual per-sample timestamps are stored in a data header, per sample.
Previously, the presence of a header in the data block would indicate the start of a new segment, and methods would look for the header to start a new memmap. That previous solution, when applied to millions of headers in the PTP-enabled files, led to OS-level problems by creating too many memmaps.
This PR allows the new PTP-enabled NSx files to be loaded.
Note: the time range of data files no longer start at 0! They start at whatever the monotonic clock was when the NSx file was recorded. We can't simply re-zero the timestamps per file because there might be multiple sets of files that were recorded simultaneously (i.e., from different pieces of hardware) but didn't start at the exact same time. We could re-zero with the common minimum across all devices, but the BlackrockRawIO loader doesn't support multi-device ns6 files. For now, it's best to leave it to the user to load the K files manually then find their own common t-zero among those K files.