-
Notifications
You must be signed in to change notification settings - Fork 16
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
000458/FlatironInstitute #101
Conversation
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.
Overall looks great! I just made a few small suggestions
Co-authored-by: Paul Adkisson <paul.wesley.adkisson@gmail.com>
Co-authored-by: Paul Adkisson <paul.wesley.adkisson@gmail.com>
Thanks @pauladkisson I committed those changes. |
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 the changes, looks good!
@yarikoptic can this be merged? |
fix the typo?
meanwhile let's checkout the content... |
"source": [ | ||
"# This notebook compares the time taken to stream an NWB file from DANDI archive\n", | ||
"# using lindi and fsspec. On my laptop on my home WiFi network, lindi streaming\n", | ||
"# took 7.52 s and fsspec streaming took 141.01 s. This is expected to depend\n", |
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.
any chance to add timing comparison to your nwb-tuned alternative to fsspec for a more frank comparison? ;-)
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 would expect lindi to be about the same as remfile, but I'm moving toward using lindi for everything since it has benefits beyond just this streaming. The purpose here is not to do a rigorous comparison between the methods, it's just to motivate why I'm not using fsspec, which seems to be the standard recommended method in all the docs still. So I'd prefer not to also include remfile here.
white circle: 316 valid and not running | ||
ESTIM TARGET REGIONS | ||
MOs: 1103 valid and not running | ||
n/a: 316 valid and not running |
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.
FWIW, could have that script could have (optionally?) produced markdown with neurosift URLs pointing to those particular files/settings in neurosift to select the desired trials? or URL would be a bit unruly to create and fragile (might change in the future)?
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.
Great idea. It's now markdown with links to NS. Not as fancy as the settings as you suggest, but it gets you to the file.
- ipykernel | ||
- matplotlib | ||
- lindi==0.4.0a1 | ||
- fsspec |
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 wonder if might be worth to version all of them to guarantee reproducibility/reliable operation (eventually dandi or pynwb api could break etc)? or may be provide complimentary environment-frozen.yaml
?
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've added the versions
besides the typo looks great to me and I verified that .md instructions work as described! Left some ideas, but in general after typo is fixed IMHO could be merged. |
Thanks @yarikoptic I made the updates. |
Adds 000458/FlatironInstitute
It goes along with this blog post to show how to use Neurosift to explore the PSTH plots for this Dandiset.
Includes:
Also includes 000_lindi_vs_fsspec_streaming.ipynb to motivate why lindi is used instead of fsspec for streaming the NWB files.
@pauladkisson @bendichter @h-mayorquin