-
Notifications
You must be signed in to change notification settings - Fork 261
Cheetah v5.6.0 fixes only #1257
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
Cheetah v5.6.0 fixes only #1257
Conversation
# for purpose of dtypes, features in the file are always fixed 8 presently, | ||
# whether mentioned in the header or not. Features may not be listed in the header | ||
# if no feature names are assigned in Neuralynx software. | ||
nb_feature = 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @PeterNSteinmetz Hard coding the number of features, tetrode channels and number of samples per waveform here seems risky, are you sure this is really true for all Neuralynx files ever recorded? Maybe it would be better to use the default values only if that information can not be retrieved from the info dictionary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These number are part of the hard definition of the file format as provided by the manufacturer. These are the maximum numbers possible. The normal result of assuming 8 when a smaller number are used would be to have some features all 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the same is true for the waveform shape? It's not possible to record fewer samples per waveform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, .Nse records are always of fixed length. Again I think some of the latter part of the record could just be 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case of 0-padding of the waveform it would still be more user friendly to return only the non-padded waveform data, so to use the indicated waveformshape and not the default of 32 samples, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Nse files are always 32 sample points. I am not aware of anything in the files documenting more or less than 32 being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since that information is in the header for at least some files, it might be possible to adjust that setting in some files. It might still be safer to use that information if available and only use the default when no info on this is present in the header. @samuelgarcia Do you have an opinion on that? See also lines 863 and 866 below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that there is any information in the headers of any files on there being fewer than 32 sample points.
The features are a separate item. I also don't know where the code which counts them and you note above came from. There are a lot of files from older versions which have features embedded but no mention of them in the headers. I think that was a more recent addition to the headers. And such changes are again undocumented by Neuralynx so one would have to guess at which versions include it or not based on extant examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this for the provided dataset.
Feel free to start a PR on this :) |
Can this be merged? @JuliaSprenger @samuelgarcia ? |
The fairly small fixes to handle issue #1202. Warning, this does not include the test data or code because we could not get permission to include the data.
In truth the whole handling of the date format parsing should be generalized a bit.