Skip to content
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

Fix PR111 and RED frame bugs #114

Merged
merged 4 commits into from
Mar 22, 2025
Merged

Conversation

Baharis
Copy link
Contributor

@Baharis Baharis commented Mar 20, 2025

The context

I've been working on some cool features that will IMO improve the depth and with of instamatic. However, I realized that with a month worth of work my planned PR will be very unfocused and overwhelming, so I decided to split it into a few chunks.

Bug 1

I have recently found three bugs, two of which were introduced in my PR #111. There, I suggested wrapping get_media (new generalized get_image and get_movie) in camera.block() and camera.unblock() statements which avoids collecting several "0-second" preview frames. I did not think though that unblocking once cancels both blocks:

ctrl.cam.block()                    <- working block
ctrl.cam.get_image()
|   while experiment_lasts:
|       ctrl.cam.block()            <- fancy no-op
|       [get image from grabber]
|       ctrl.cam.unblock()          <- unblocks so preview briefly re-starts grabbing 0.05s frames!
ctrl.cam.unblock()                  <- another fancy no-op

This is an issue because every data-collection frame using block/unblock will now be interrupted by some previews when collecting images, as evidenced by adding a log statement and running a RED frame. Given most instamatic frames use get_image to collect movies, this is a big issue as it introduces unnecessary delays between frames:

2025-03-20 15:09:21,236 | red_frame:142 | INFO | Start RED experiment
...
2025-03-20 15:09:22,410 | camera_simu:54 | DEBUG | Collecting image with exposure=0.05
2025-03-20 15:09:22,462 | camera_simu:54 | DEBUG | Collecting image with exposure=0.5    <- RED image
2025-03-20 15:09:22,964 | camera_simu:54 | DEBUG | Collecting image with exposure=0.05   <- previews collected 
2025-03-20 15:09:23,016 | camera_simu:54 | DEBUG | Collecting image with exposure=0.05      despite block() call
2025-03-20 15:09:23,068 | camera_simu:54 | DEBUG | Collecting image with exposure=0.05
2025-03-20 15:09:23,121 | camera_simu:54 | DEBUG | Collecting image with exposure=0.5    <- RED image
2025-03-20 15:09:23,624 | camera_simu:54 | DEBUG | Collecting image with exposure=0.05
2025-03-20 15:09:23,676 | camera_simu:54 | DEBUG | Collecting image with exposure=0.05
2025-03-20 15:09:23,728 | camera_simu:54 | DEBUG | Collecting image with exposure=0.05
2025-03-20 15:09:23,780 | camera_simu:54 | DEBUG | Collecting image with exposure=0.5
2025-03-20 15:09:24,285 | camera_simu:54 | DEBUG | Collecting image with exposure=0.05
...

I addressed this bug by replacing block/unblock pairs with a new blocked context. If you wrap some code in with cam.blocked(): now, the cam will block at start of the wrapped code and restore to the previous state, whether blocked or not, at the end. As an added value, if blocking camera for a short time, you can use a nice blocked context as a "re-entrant" event:

self.unblock()
with self.blocked():         <- remembers: was unblocked before
    do_stuff()
    do_other_stuff()
...                          <- was unblocked before, so unblocks.

BUT:

self.block()
with self.blocked():         <- remembers: was blocked before
    do_stuff()
    do_other_stuff()
...                          <- will remain blocked after event

Bug 2

PR #111 introduced int_nm and float_deg to a new instamatic.typing. This typically works OK but in some specific cases statements like from typing import Callable may try to find Callable in instamatic,typing. I don't fully understand the mechanism behind it, I managed to get it in a very unique conditions, but to avoid any potential issues I just suggest moving instamatic.typing to instamatic.typing_.

\path\to\instamatic\venv\Scripts\python.exe \path\to\instamatic\src\instamatic\main.py 
Traceback (most recent call last):
  File "\path\to\instamatic\src\instamatic\main.py", line 9, in <module>
    from typing import Callable
  File "\path\to\instamatic\src\instamatic\typing.py", line 6, in <module>
    from typing import Annotated
ImportError: cannot import name 'Annotated' from 'typing' (consider renaming '\\Path\\to\\instamatic\\src\\instamatic\\typing.py' since it has the same name as the standard library module named 'typing' and prevents importing that standard library module)

Bug 3

While investigating an old-but-reliable RED frame I realized that some of the logging statements present there were either misleading or wrong, so I fixed a few of them; see change log for details.

Bugfixes

  • MediaGrabber: fix bug: get_media cleared continuousCollectionEvent, allowing premature collection of preview images;
  • instamatic.typing: move to instamatic.typing_ to avoid rare but potential name clash with built-in module;
  • RED Experiment, acquire_data_RED: fix broken and improve misleading log statements.

Effect on the codebase

This PR fixes issues introduced in #112 and fixes minor logging issues in RED frame. No functionalities change, but a new with cam.blocked() statement is now available to block camera for short periods of time.

@Baharis Baharis self-assigned this Mar 20, 2025
@Baharis Baharis requested a review from stefsmeets March 20, 2025 16:14
@Baharis
Copy link
Contributor Author

Baharis commented Mar 21, 2025

Relevant PEP that suggests import typing should be unambiguous in Python 3 but ¯\(ツ)/¯.

Copy link
Member

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow-up and the fix! Glad to see these small but consistent improvements.

The only change I'd suggest is to rename to _typing (to indicate a private module) or types (if it is intended to be public) instead of typing_. I'm not a huge fan of trailing underscores in module names.

@Baharis
Copy link
Contributor Author

Baharis commented Mar 22, 2025

@stefsmeets I personally do not mind neither of the options, private _typing makes sense though since there should rarely be a point for someone to import our specific type hints. Added and will merge as soon as tests succeed.

@Baharis Baharis merged commit a1d1a9a into instamatic-dev:main Mar 22, 2025
7 checks passed
@Baharis Baharis deleted the pr111_bugfixes branch March 22, 2025 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants