Small improvements to the napari plugin package #52
Conversation
| f"stream {stream_id} frame {f.metadata().frame_id}" | ||
| ) | ||
|
|
||
| # TODO: (nclack) fix this awkwardness. |
There was a problem hiding this comment.
I think this should be fixed by extracting this code block into a function, since variables in Python are functionally scoped.
| @@ -204,7 +196,23 @@ def is_not_done() -> bool: | |||
| runtime.stop() | |||
| logging.info("STOPPED") | |||
|
|
|||
| viewer.layers.clear() | |||
There was a problem hiding this comment.
Handles multiple runs with different stream counts without weirdness.
| p.video[0].camera.identifier = dm.select(DeviceKind.Camera, cameras[0]) | ||
| p.video[0].storage.identifier = dm.select(DeviceKind.Storage, "Trash") | ||
| # p.video[0].storage.settings.filename = output_filename | ||
| p.video[0].camera.settings.binning = 1 | ||
| p.video[0].camera.settings.shape = (2304, 2304) | ||
| p.video[0].camera.settings.shape = (64, 64) |
There was a problem hiding this comment.
I arbitrarily picked a small simulated radial sine image as a quick way to test things out. Don't have a strong preference about what the simulated image should be here.
| def gui( | ||
| viewer: "napari.Viewer", | ||
| frame_count: int = 100, | ||
| ): | ||
| stream_count: Literal[1, 2] = 2, |
There was a problem hiding this comment.
I added this argument to allow exercising both the 1 stream (after fixing it) and 2 stream setup code. Alternatively, we could just remove the 1 stream setup.
There was a problem hiding this comment.
I'd love to remove the setup functions. I think they might be used in the tests in places, which isn't great. They really shouldn't get exposed in __init__.
Shouldn't hold up this pr though.
|
Also, do we need to maintain the |
python/acquire/__init__.py
Outdated
| logging.info("REUSING RUNTIME") | ||
| return _g_runtime | ||
|
|
||
|
|
||
| # TODO: (nclack) add context manager around runtime and start/stop |
There was a problem hiding this comment.
I was tempted to tackle this TODO too, but I guess this needs to be done in runtime.rs?
| """The global acquire runtime.""" | ||
|
|
||
|
|
||
| def _get_runtime() -> Runtime: |
There was a problem hiding this comment.
I moved this up here to be next to the declaration of g_runtime and also because this function doesn't capture anything from the definition of gui.
| @@ -8,13 +10,32 @@ | |||
|
|
|||
| import logging | |||
|
|
|||
| FORMAT = ( | |||
| if TYPE_CHECKING: | |||
| def gui( | ||
| viewer: "napari.Viewer", | ||
| frame_count: int = 100, | ||
| ): | ||
| stream_count: Literal[1, 2] = 2, |
There was a problem hiding this comment.
I'd love to remove the setup functions. I think they might be used in the tests in places, which isn't great. They really shouldn't get exposed in __init__.
Shouldn't hold up this pr though.
| @@ -17,4 +17,4 @@ def setup_two_streams(runtime: Runtime, frame_count: int) -> Properties: ... | |||
|
|
|||
| g_runtime: Optional[Runtime] | |||
|
|
|||
| def gui(viewer: "napari.Viewer", frame_count: int = ...): ... # type: ignore | |||
| def gui(viewer: "napari.Viewer", frame_count: int = ..., stream_count: int = ...) -> None: ... | |||
There was a problem hiding this comment.
Should this be exposed in the pyi?
I almost want gui() to be hidden - only exposed as a napari plugin hook.
| "%(levelname)s %(name)s %(asctime)-15s %(filename)s:%(lineno)d %(message)s" | ||
| ) | ||
| logging.basicConfig(format=FORMAT) | ||
| logging.getLogger().setLevel(logging.INFO) | ||
|
|
||
|
|
||
| g_runtime: Optional[Runtime] = None |
There was a problem hiding this comment.
Is there a better pattern to use for naming/using this singleton?
The original name g_runtime is from a convention I use for C code, but it seems like it would be better to use an underscore prefix to indicate it's private in python.
|
Hi @andy-sweet, could you look at #60 and see if that gets fixed here? If so, feel free to merge. |
I'm ramping back up on this project and was using the napari plugin as an initial driver. I noticed some TODOs (e.g. scope awkwardness) and some broken things (
setup_one_streams), so decided to fix them. I also made some other refactors and typing additions along the way that are more subjective.