-
Notifications
You must be signed in to change notification settings - Fork 249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NeuralynxIO: partial support for NVT files #1480
base: master
Are you sure you want to change the base?
NeuralynxIO: partial support for NVT files #1480
Conversation
…iple values feat: add header support for Neuralynx nvt files
Only loading dnextracted_x, dnextracted_y and dnextracted_angle as 3 dimensionless signal channels. Only tested on directories with single nvt file.
…s as literal string of a list
…iles for NeuralynxIO and NeuralynxRawIO
…iple values feat: add header support for Neuralynx nvt files
Only loading dnextracted_x, dnextracted_y and dnextracted_angle as 3 dimensionless signal channels. Only tested on directories with single nvt file.
…s as literal string of a list
…iles for NeuralynxIO and NeuralynxRawIO
…n-42/python-neo into feature-neuralynxrawio-nvt
Thanks @LEGion-42, we will review soon. :) EDIT: See also that tests are currently failing. |
pytest somehow skipped all the necessary neuralynxrawio tests for me so I didn't catch these before submitting the PR. Sorry for the oversight. :( The 4 AssertionError's are my code working as intended and does not cause problems -- the test simply did not consider the fact that I'm passing behavior data as signals. The header size of 4 is just the original 1 from an ncs file + 3 new ones from the nvt file. I have now updated the test with proper values. The KeyError on the other hand raises some questions. I implemented a temporary fix by duplicating the nvt data to every segment to gether with a warning message telling users that nvt file should be loaded independently if there are multiple segments. But I think a proper fix should be allowing different segments have different channels? |
That is likely what we would do. I guess I don't know NVT/Neuralynx well enough. Segments for neuralynx can be due to gaps in the signal. So my questions would be 1) are there not gaps in the nvt that we need to account for? 2) can you have behavioral input of some sort for one segment realistically and not for others? You wouldn't happen to have some easy to access docs for the neuralynx format would you? |
@PeterNSteinmetz As for the sampling rate, since nvt files are video tracking data, the fps is typically around 30. |
@LEGion-42 . Thanks that sounds good. The test files are really just to test that headers are being read properly and records interpreted correctly. Normally we trim them down to a smaller number of records and use a small number of them. If there are no unusual features like gaps in the files, that is fine. The files can be anonymized if needed to remove any information, such as time/date, which might be identifiable to an individual person. I agree the video tracking data should be in the same segment as per this design. Given the separate sampling frequency it should be in a separate stream. |
@PeterNSteinmetz I got permission to share our nvt data. However our nvt files have size ranging from 30 to 100mb, which are all outside of the 10mb file size requirement on gin and I have no idea how to trim them down. Any suggestions? |
@LEGion-42 Great. We don’t need an entire recording set, just perhaps 3-6 files. Since the header parsing is of primary interest, please trim to just header plus 10-20 records each. If you are on MacOS or another Unix system you can use the ‘truncate’ command to shorten them. I usually add a ‘_trunc’ suffix to the file name before the extension to indicate this has been done. You can compute the target size as 16384 bytes + number of records x record size. The record sizes for the various file types are available in the document at https://neuralynx.com/_software/NeuralynxDataFileFormats.pdf if you have the ephy-testing data checked out, please create an issue at the gin site. Then add your files to an appropriately named sub folder of Neuralynx. |
@zm711 Zach, what do we need to do to get some of the pull requests for corresponding test data merged over there? |
Hey Peter. Are you talking about from the beginning? We have instructions on our repo how to make a PR on gin. If you're talking about merging test data just @ one of us and we can review it and merge it. We like to check to make sure it's not too big and that everything looks to be in order before we merge it. :) |
Hi Zach, well there is this one https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/116 . That provides test data from some code that can be used to deal with some of the header issues in this pull request. |
@LEGion-42 If you would like some further assistance with getting the test files ready, just let me know. |
@PeterNSteinmetz Sorry for no being responsive, I was quite busy the past week. I'll try to upload the nvt data today. |
@zm711 @PeterNSteinmetz Here are the the nvt files. gin wouldnt let me pass the their captcha for some reason (maybe im a bot?). So can you guys upload them? Sorry for the trouble. |
I am in the Tetons now and can take a look at these about July 2.
Thanks,
Peter
---
***@***.***
|
@LEGion-42 These downloaded fine for me. I will try and structure the folder, write some tests, and start a pull request in the next few days. |
@PeterNSteinmetz, it is merged. I would ping us here on github in your PR (probably a couple times) so we review and merge it. Thanks for all your help on these PRs! |
Actually @PeterNSteinmetz, that was a broken file. I had to overwrite and erase your PR (gin can be annoying). You submitted a stub on your branch rather than a file, but since I had merged it I had to delete it. |
Thanks Zach. @zm711 I am going to move discussion of that dataset over to its pull request at https//gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/116 . |
We are still working on getting your test file onto g-node/gin. So that is in the works. There was another PR for neuralynx where we were working on simplifying the parameters moving forward. That has now been merged which has caused conflicts in your PR. Would you like to fix those conflicts while we working on adding the test files. |
@zm711 I'll take a look on the weekend. Thanks for letting me know. |
@LEGion-42 I am about to prepare the push of the shortened .nvt files you provided. But I am wondering if a directory structure where all 5 .nvt files are in one directory would be good? This would allow testing of providing a directory to the constructor, which should then produce 5 channels, versus individual file names which should produce one. Would such a structure provide any difficulties for your code or tests? |
@PeterNSteinmetz currently the code only supports 1 nvt files per directory. Multiple nvt files won't crash the code but will cause problems. |
@LEGion-42 OK, thanks, good to know. I will move them to the separate directories then before committing them. |
@LEGion-42 How would you like the Readme.txt file to read? Normally we state where the files are from and who created them. |
@LEGion-42 These files have been added to a pull request in the test data https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/141. Once incorporated there, it should be possible to add some tests to the code to support this addition to the functionality. Thanks for your work on this. |
just one quick confirmation: these files are either a dummy recording or simulated data right? We just don't want private files on the public testing repo (for example in the case of human recording/medical data). Could you confirm how this data was obtained and then we will merge the testing data! |
@zm711 Data was collected from mice and was explicitly allowed for release by my supervisor so don't worry! |
Thanks so much for confirming. The testing data has now been merged into our repo! So test away :) |
@LEGion-42 I was just working a bit on the header parsing and I noticed something about about the headers in these .nvt files. They do not contain a first line '######## Neuralynx Data File Header' . This is the only case we have seen where this is missing. That brings up some questions: Are these produced by a Neuralynx recording system? If so, which one and versions of the software. The header does say CheetahRev 5.6.0. But this just seems quite different. It makes me wonder if there is some configuration difference there on that system what was not intended. Implementing this will require us to either drop the requirement for that first line as a check generally or perhaps only omit it if the FileType is Video. |
I just checked the nvt files (both on my end and the pull request) and the headers including the first lines and the CheetahRev's are all there? Can you please recheck? https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/141/files here you should see that they all have properly formatted headers |
@LEGion-42 You know, you are right. I don't know what I got confused. Sorry for the false alarm. Glad we have the correct files for testing in the ephy_testing_data now. |
:D don't worry about it. Glad that the confusion was cleared. |
Type of Change
Description
Added partial support for Neuralynx NVT files (#1463). for
NeuralynxIO
andNeuralynxRawIO
, the x and y pixel coordinates and animal head angle from the nvt file are treated as dimensionless analog signals bundled into a signal stream separate from the NCS stream.NlxHeader
can now read NVT files as well. For detailed output please refer to the Testing section below.A buggy regex in
NlxHeader
was also fixed. Previously it can only read a single value after the header keys, now it can read multiple.Limitations
As a temporary solution to the potential problems these limitations might cause, I added an optional
ignore_nvt
argument to theNeuralynxIO
andNeuralynxRawIO
classes. When enabled,NeuralynxIO
andNeuralynxRawIO
will behave like how they were before my changes.NlxHeader
works fine without any problems during testing.Testing
Code was tested in accordance with the official contribution guideline. Additionally, it was tested on my own lab's data. Listed below are some examples.
Each of the
VT1
signal channel represents a data field (x, y pixel position and head angle).First column is x posiiton, second y position and thrid head angle in degrees.
Video resolution from the NVT file header is passed into
array_annotations
for easy access. However because list type inarray_annotations
is buggy, it is passed as a literal string ('[720 480]'
) and not a list. Though once it is accessed it can be easily converted into a list usingast.literal_eval()
in theast
package.Additional Information
I should be able to make semi-regular contributions to this project. I don't have a good solution in mind to include the dwPoints and dntargets data fields as of now, but I should be able to fix the problem with loading multiple NVT files and implement proper segment assignment in the near future.
I opened this pull request dispite the above mentioned limitations because I think in most cases (at least in my lab and other labs I know of) there are only one NVT file in a given directory, and the files often share the same time basis (since they are likely from the same experiment session). So this current version should be helpful enough in most scenarios. With the
ignore_nvt
argument, users will also be able to revert back the code's behavior. So even if the new code causes problems, they could be easily avoided.