-
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
Axonarawio #958
Axonarawio #958
Conversation
Hello @sbuergers! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-04-09 18:41:24 UTC |
Hi steffen, |
Hi Samuel, thanks a lot for the review! I wasn't planning on including the notebook in the final PR, sorry for including it in the first place, causing confusion. I had checked the timing of _get_analogsignal_chunk() earlier and that seemed reasonable (and I could not make it speed up more). For instance, reading 2 million samples from 16 channels took ~1.2s, reading 10 million samples from 16 channels took ~11s. This is ~1.3Gb. This is using an external HD and a mediocre laptop. I am not familiar with logger, and |
@samuelgarcia can you have a look if this is good now, please? :-) |
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.
Looks good to me!
Steffen and Sam do some debug.
@samuelgarcia The tests passed earlier, but I had to do some final Pep8 adjustments (spaces between arithmetic operators, some empty lines to be deleted or added. As far as I can tell, this is done for now. If we want to make additional adjustments like adding tracking data in the future it would make sense to create a new PR for it. |
Hi Steffen. Thanks very much for this PR. It is OK for me. |
Thanks @samuelgarcia Is it customary to delete the branch now? |
I think you have to delete the branch in your repo by your own. |
''' | ||
|
||
# Get useful parameters from .set file | ||
params = ['rawRate'] |
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.
Just was checking w/ Abid on this project and it popped up in my head. There is this looming bug in the Axona software where it will save the sample rate at 48kHz even if the end-user recorded in their 2ms spike (24kHz) mode. You have to do an additional check for the Spike2msMode
parameter. If the value is 0, then it is 48k, else 24k.
An additional caveat, I saw that Abid sent you a 2ms mode file that was just converted from the RAW mode. That converter I think will just dump the same .set
file, so 2ms mode will not be set to 1 (thus the above check isn't even true in the case where the end-user wants to chunk their data from the raw mode to the 24k mode, although I'm not sure why they would want to).
Could try to just infer sampling rate from the timestamps of the .N
files. I think a majority of the end-users use the 48k mode though.
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 Geoff,
Thanks a bunch for pointing this out! We will take it into account!
Cheers,
Steffen.
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.
No problem!
After more thinking, I suppose there is a valid case for converting from RAW (.bin) to 2ms mode since the Tint
software that the end-users of this data-type use requires the "Tint" format vs the "RAW" format. So it could be that they would want to visualize 2ms of spike chunks in the software. Again, I think most people use the 1s chunk (48kHz) mode.
Keep up the good work, I'm excited to see the results of this pipeline.
Best,
Geoff
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 @GeoffBarrett. Thanks for the feedback. I updated the current AxonaIO PR accordingly: #986
Now the Spike2msMode
is first taken into account to determine the sample rate and only if this fails the .N
file header sample_rate
is used. For now I am not trying to extract the sample rate from the timestamp resolution as I think this method also won't be 100% reliable, depending on the number of spikes extracted and the underlying precision of the timestamps.
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 @ GeoffBarret, if you are sure about the fact that in special cases the parameters extracted from the header as in #986 can still be wrong we can also raise a warning. If you have an idea how to identify if this is the case I would be happy to add this to the open PR. However, I would hesitate to add a global warning that is always displayed, as this will be most likely ignored by the users.
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.
@JuliaSprenger you're right, the timestamps in the .N
would just provide the time that the signal went above threshold, so they wouldn't really allow you to infer sampling frequency. I think the best you'd be able to do is read that Spike2msMode
. I haven't used the pipeline, but potentially the end-user could provide a sample frequency override kwarg, not sure if it's some sort of CLI and the users can provide parameters or something.
It is definitely an issue that I brought up with Jim Donnett a few years back. If you look through the header info in the 2ms data that Abid has provided you will see the 48k (as well as in the .set
) so the issue is definitely there. I don't have any 24kHz recorded data, but you could always reach out to Abid and have him record a minute of 24kHz data (not converted from .bin
), and look at the headers.
Draft for axonarawio.py (which is still very much in development).
Note that the draft still includes a jupyter notebook
read_axona_bin_cont.ipynb
that I use for testing code, which is in this repo for my convenience. But once we actually want to merge to master this has to be deleted first.