Aharoni-Lab / miniscope-io

Data formatting, reading, and writing from miniscopes
https://miniscope-io.readthedocs.io
GNU Affero General Public License v3.0
6 stars 2 forks source link

Feature - Battery Log, Data Formats, Plots #8

Closed sneakers-the-rat closed 12 months ago

sneakers-the-rat commented 12 months ago

New features:

Reverted:

t-sasatani commented 12 months ago

grab_frames notebook is restored to using the example data and having the evaluated output present

The common usage is directly reading SD cards so I think that should be available in the notebook. Although I understand it's better to have a running example with data in the repo is better. So I think we should leave an exposed option for selecting external drives.

sneakers-the-rat commented 12 months ago

The notebook is just an example, so someone using the code could replace the path which could be a path to a drive, we could just add a comment like you had explaining that explicitly in case it's unclear

sneakers-the-rat commented 12 months ago

if we wanted to make the notebooks more than examples, like the intended way to use the package, then I have made a widgets module that we could add a file selection dropdown for, and also take the example in read_frames which makes an ipywidgets video player as well and make it reusable, but ya probably shouldn't hardcode local variables in the repo

t-sasatani commented 12 months ago

That makes sense if just replacing it with a device path works, but there was a minor problem resolving the device path (https://github.com/Aharoni-Lab/miniscope-io/issues/6), which requires a tiny modification in the later function call. As most initial use cases will directly read SD cards, I think it makes sense to be a little more oriented around that.

sneakers-the-rat commented 12 months ago

huh, don't know why that would be the case, resolve should only change the path if it's not an absolute path, but we can just take out the resolve call, usually it's neutral and just to make problems from using relative paths explicit, but sure we can take that out

t-sasatani commented 12 months ago

That makes sense. Would you like to include that commit in this PR or another PR?

sneakers-the-rat commented 12 months ago

Here, i'll merge this then make that its own PR, 1 sec