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

Snap button widget #147

Merged
merged 39 commits into from
May 19, 2022
Merged

Snap button widget #147

merged 39 commits into from
May 19, 2022

Conversation

fdrgsp
Copy link
Contributor

@fdrgsp fdrgsp commented Mar 10, 2022

Creating a new general purpose Snap QPushButton widget linked to the pymmcore-plus snap method.
Once the button is clicked, an image is snapped and the pymmcore-plus imageSnapped signal is emitted.

@fdrgsp
Copy link
Contributor Author

fdrgsp commented Mar 11, 2022

I wanted to modify the snap method in the main_window like below:

sig.imageSnapped.connect(self._snap)

def _snap(self, img: np.ndarray):
        self.stop_live()
        create_worker(self.update_viewer, img, _start_thread=True)

but when the image is snapped and the ndarray is passed to the update_viewer function I get this error:

WARNING: QObject::connect: Cannot queue arguments of type 'QVector<int>'
(Make sure 'QVector<int>' is registered using qRegisterMetaType().)

Do you know why?

@tlambert03
Copy link
Member

Yeah that suggests your communicating with the gui across threads. Updating the napari viewer should never happen in a non main thread. You need to connect the viewer update to a callback after the worker is finished

Comment on lines 227 to 229
def _snap(self, img: np.ndarray):
# update in a thread so we don't freeze UI
create_worker(self.update_viewer, img, _start_thread=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def _snap(self, img: np.ndarray):
# update in a thread so we don't freeze UI
create_worker(self.update_viewer, img, _start_thread=True)
def _snap(self, img: np.ndarray):
self.stop_live()
# run snap in a thread so we don't freeze UI
create_worker(
self._mmc.snap,
_start_thread=True,
)

This can't just update the viewer - it also needs to trigger a new snap so that it works for snap_on_click

Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably remove this entirely in the future once the new stage widget comes in

Copy link
Contributor

Choose a reason for hiding this comment

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

@fdrgsp this still needs to be changed - currently snap on click is broken

@ianhi
Copy link
Contributor

ianhi commented Mar 14, 2022

@fdrgsp can you add some tests - even a super basic one like in https://github.com/tlambert03/napari-micromanager/pull/150?

After that I'm happy to give this a more thorough review

@ianhi
Copy link
Contributor

ianhi commented Mar 14, 2022

also, rebasing on main should fix the test failures

@codecov
Copy link

codecov bot commented Mar 14, 2022

Codecov Report

Merging #147 (0024fe0) into main (e411f45) will increase coverage by 0.27%.
The diff coverage is 90.38%.

@@            Coverage Diff             @@
##             main     #147      +/-   ##
==========================================
+ Coverage   81.56%   81.84%   +0.27%     
==========================================
  Files          25       26       +1     
  Lines        2268     2314      +46     
==========================================
+ Hits         1850     1894      +44     
- Misses        418      420       +2     
Impacted Files Coverage Δ
micromanager_gui/main_window.py 69.56% <50.00%> (+0.12%) ⬆️
...romanager_gui/_core_widgets/_snap_button_widget.py 95.45% <95.45%> (ø)
micromanager_gui/_gui_objects/_tab_widget.py 93.67% <100.00%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e411f45...0024fe0. Read the comment docs.

@fdrgsp
Copy link
Contributor Author

fdrgsp commented Mar 17, 2022

@fdrgsp can you add some tests - even a super basic one like in #150?

After that I'm happy to give this a more thorough review

@ianhi basic test added!

Copy link
Contributor

@ianhi ianhi left a comment

Choose a reason for hiding this comment

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

Looking good! But still need to fix snap on click.

The only final thing to consider would be to generalize the snap button widget to work for any camera, not just the default camera (e.g. see how #150 and #142 manage this). But this is a good PR otherwise and we can always expand that later.

tests/core_widgets/test_snap_button_widget.py Outdated Show resolved Hide resolved
tests/core_widgets/test_snap_button_widget.py Outdated Show resolved Hide resolved
micromanager_gui/main_window.py Outdated Show resolved Hide resolved
Comment on lines 227 to 229
def _snap(self, img: np.ndarray):
# update in a thread so we don't freeze UI
create_worker(self.update_viewer, img, _start_thread=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

@fdrgsp this still needs to be changed - currently snap on click is broken

@fdrgsp
Copy link
Contributor Author

fdrgsp commented Mar 20, 2022

test error due to codecov/patch — 74.07% of diff hit (target 80.00%)

Copy link
Contributor

@ianhi ianhi left a comment

Choose a reason for hiding this comment

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

One comment on a small improvment - but on the whole this looks good

micromanager_gui/main_window.py Outdated Show resolved Hide resolved
@ianhi
Copy link
Contributor

ianhi commented May 19, 2022

@tlambert03 I think this is ready and would be a nice to have as a separate widget for the demo. Any chance of merging this todayish?

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

good stuff. Thanks!

@tlambert03 tlambert03 merged commit 1bad737 into pymmcore-plus:main May 19, 2022
@fdrgsp fdrgsp deleted the snap_button branch May 27, 2024 18:52
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.

None yet

3 participants