Closed t-sasatani closed 2 months ago
Thanks @sneakers-the-rat , I never used pydantic before but this was useful. I made some updates (described in commit messages) and confirmed it works with the v0.2.0 firmware. I also noticed this PR partially addresses #19.
No rushes at all, but I think we can merge this so I flagged this as ready for review.
Gimme a day or two. Strike has entered a weird status where we are half on strike but I will do a review of this when I get a second
in https://github.com/Aharoni-Lab/miniscope-io/pull/21/commits/9c7797e348d2bd04def4391660c2e5735431d1ac I moved the example config files to a data
directory and made top-level consts to refer to those directories. there is already a miniscope_io.config
module, and we might want to eventually expand that into a package, so trying to both avoid name clashes and allow future expansion for additional static files we want to distribute along with the package.
I was unsure of the status of these config files - are they just examples? or are they the "defaults" we want to ship for a given set of devices? that affects whether we want to distribute them with the package (eg. put them in the examples
directory in the repo root vs. within the package and available when one pip installs it) and how we want to structure them (one default per device, etc.)
Here's a big refactor trying to make these models more generic and starting the process of unifying them with the existing models, changes described in the commit: https://github.com/Aharoni-Lab/miniscope-io/pull/21/commits/5699dae077dae758e7154923cf220c34530679ad
See https://github.com/Aharoni-Lab/miniscope-io/pull/21/commits/a8e1eaada4ddc9b51a7db0397866a1a4955b574a for how to encapsulate behavior like coercion in the model using validators so we aren't doing work in the classes that use the config to fill in missing/improperly formed values. more explicit and testable (see added tests for parsing hex values from strings and ints in yaml) - all new behavior shoudl have accompanying tests from now on.
That commit also removes the implicit setattr
calls and replaces them with direct access to self.config.{value}
so that type checkers stop complaining about undeclared values and we can make maintenance easier as these models get complex - now there is an explicit connection between the model and the values used in the daq class that can be understood by static type checkers.
with that i'm good to go - I do think that we need mocks to test this ASAP because i can't tell if i broke anything here, and that is going to be the case for this code from now on. if we want to accept external PRs we need to make sure they don't break things. we can be relatively certain that I didn't break the SDCard reader even through a relatively large refactor because it's tested (and if i did break something, there is a straightforward path to diagnose, bugfix, and fill the missing holes in the tests)
before we merge can you bump a minor version, write notes in the changelog, and make sure that the intended usage of the .yaml config is reflected in the docs? there are a few outstanding docs needs here, but they are mostly bc we need to decide on a more macro level usage pattern - eg. how are people supposed to supply their own config files? we have the notion of a user directory but aren't using it atm. for the sake of this PR if we just make sure that the relationship between the model and the config .yaml is clear.
I turned on PR docs builds, so you can see the build here: https://miniscope-io--21.org.readthedocs.build/en/21/
the whole thing is pretty messy, but if it would be possible to take 5-10 minutes to make this module's API docs look nice, or to add some user guide docs for how this is intended to be used then that would be nice :)
Thanks, I skimmed through the code, and the refactored structure made more sense. I'll probably have time to go through your comments and merge this weekend or early next week.
I agree about the test too. I was thinking about doing this next PR because it's the first streamDAQ version that is close to what the actual interface might look like, and I have a functional physical setup now.
Hey
I seem to have problems just running the stream_daq.py file after setting up the environment. Not sure if this is a mistake ion my side or whats going on.
here the link to an issue I opened with the error message. https://github.com/Aharoni-Lab/miniscope-io/issues/22
@MarcelMB re: https://github.com/Aharoni-Lab/miniscope-io/pull/21/commits/40f1fba5d7735cafaa2ad1242165fd2d83b09cd0
Please see discussion above about why we might not want to do this. Manually unpacking the config object makes the object more brittle, though doing it this way is better for static analysis than using setattr
. If we are using external config, we either a) want to have a routine outside __init__
that unpacks it and passes as kwargs, or b) keep the config object intact and use it - we want to keep the link between the config and the instantiated object as clean as possible. Changes like this add up and make the package hard to understand.
We also dont want any print statements in the code, and the preamble is initialized once in the init rather than continually in a loop.
Im going to revert that commit and fix using the style thats consistent with the rest of the module
see here: https://github.com/Aharoni-Lab/miniscope-io/pull/21/commits/d1ada29c136a94aef06ebf307ec348fd469a3826
self.preamble
is already cast to Bits
, and inverted according to the LSB
setting in __init__
:
the config
object starts out as a bytes
object, deserialized from either a string or a hexadecimal literal: https://github.com/Aharoni-Lab/miniscope-io/blob/d1ada29c136a94aef06ebf307ec348fd469a3826/miniscope_io/models/stream.py#L89-L96
this is tested here: https://github.com/Aharoni-Lab/miniscope-io/blob/d1ada29c136a94aef06ebf307ec348fd469a3826/tests/test_models/test_model_stream.py#L6-L22
so in https://github.com/Aharoni-Lab/miniscope-io/pull/21/commits/40f1fba5d7735cafaa2ad1242165fd2d83b09cd0 , doing this and getting the bytes
object:
and then later recasting as Bits again (and double inverting for LSB): https://github.com/Aharoni-Lab/miniscope-io/blob/40f1fba5d7735cafaa2ad1242165fd2d83b09cd0/miniscope_io/stream_daq.py#L250-L254
is unnecessary.
_fpga_recv
expects the third argument to be a bool: https://github.com/Aharoni-Lab/miniscope-io/blob/40f1fba5d7735cafaa2ad1242165fd2d83b09cd0/miniscope_io/stream_daq.py#L203
so the bytes object should not be passed there.
i reverted and fixed the initial problem in https://github.com/Aharoni-Lab/miniscope-io/issues/22
Thanks for keeping it clean and organized. Appreciate your expertise!!!!!
I'll definitely still have a bunch of errors trying to get this to run. But I also check in with Takuya on Monday to solve those before we can merge this.
Keep posting error messages! Since we dont have tests for this thats the best way to fix
`(venv) mbrosch@RELMNGYFAC31424 miniscope-io % python -m miniscope_io.stream_daq --config miniscope_io/data/config/WLMS_v02_200px.yml
Traceback (most recent call last):
File "
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/mbrosch/anaconda3/lib/python3.11/multiprocessing/process.py", line 314, in _bootstrap
self.run()
File "/Users/mbrosch/anaconda3/lib/python3.11/multiprocessing/process.py", line 314, in _bootstrap
self.run()
File "/Users/mbrosch/anaconda3/lib/python3.11/multiprocessing/process.py", line 108, in run
self._target(*self._args, *self._kwargs)
File "/Users/mbrosch/anaconda3/lib/python3.11/multiprocessing/process.py", line 108, in run
self._target(self._args, **self._kwargs)
File "/Users/mbrosch/Documents/GitKraken_mac/miniscope-io/miniscope_io/stream_daq.py", line 350, in _format_frame
if frame_buffer_queue.qsize() > 0: # Higher is safe but lower is fast.
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "
ok try now, it looks like the rest of those errors are spawned from the main process crashing. mypy says that's the last missing attribute access (and we'll fix the rest in https://github.com/Aharoni-Lab/miniscope-io/pull/23 )
Ok, the code is back to functional so I'll briefly go through formatting and merge. I moved back types for passing preambles (bytes, Bits, etc.) for now because that affects some functions using preamble length, and this is not critical anyways. It is true that this is redundant so this should be unified later.
I was unsure of the status of these config files - are they just examples? or are they the "defaults" we want to ship for a given set of devices? that affects whether we want to distribute them with the package (eg. put them in the
examples
directory in the repo root vs. within the package and available when one pip installs it) and how we want to structure them (one default per device, etc.)
I'm not sure yet. I think this depends on whether this package is going to be a stand alone io module that also gets behavioral data, etc., or if this is going to be a more dedicated io for Miniscopes that will be called from a higher-level package that gathers other data. If it's the latter, it'll make sense to be defaults that ship with devices.
Thanks for all your help. Now this PR is merged. Nice to see this is getting into shape. If some bugs appear in @MarcelMB 's environment, we can address that in the next PR.
Here to say this is super cool to read and learn from. <3 what you're all doing! You're the coolest!
Still WiP. I added the following updates for using the stream DAQ with different firmware/hardware:
I noticed @sneakers-the-rat added an interface for configuring (
config.py
) but wasn't instantly sure how to use it so I drafted it in yaml for now. Would like to know if it makes more sense to unify this PR with this interface. Will continue refining/documenting if the current way makes sense.The code will run by:
and explicitly exposing config as an input argument should be nice for working with different versions of firmware/hardware.