-
Notifications
You must be signed in to change notification settings - Fork 21
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
Stage widget #142
Stage widget #142
Conversation
this is getting closer to ready for a critical eye @fdrgsp. Particularly we want to check whether most of the logic in main_window has been preserved (though it's all removed from main_window now). might be worth checking this on a real scope. Currently, it's rather generic, so it's also likely something specific to (e.g.) autofocus may have been lost. Couple minor aesthetic issues remaining, but looking decent: |
|
||
def _on_cfg_loaded(self): | ||
self._clear() | ||
if dev := self._mmc.getXYStageDevice(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should provide an object for generic StageWidgets
and then a subclass that follows the default stage widget. Like what I did in #150 (in spirit if not in implementation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's primarily what this PR is doing. See micromanager_gui/_core_widgets/_stage_widget.py
this file here is just for the few things that are special about napari micromanager (like the QCollapsible). hence why it's in gui_objects :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this one arranges the generic stage widget into a multi widget. That could also happen in core, but the primary widget here is class StageWidget(QWidget)
in that other file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah indeed - I scanned too quickly sorry!
One test failure will be fixed with a rebase - other one looks real |
yep, this needs https://github.com/napari/superqt/pull/72 to be released |
if self._snap_on_click: | ||
self._mmc.snap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either there needs to be a change in core to have this stop live mode - or there there needs to be acheck here of if a sequence is running. This can result in this error:
untimeError Traceback (most recent call last)
File ~/Documents/oss/micro/napari-micromanager/micromanager_gui/_core_widgets/_stage_widget.py:207, in StageWidget._on_click(self=<micromanager_gui._core_widgets._stage_widget.StageWidget object>)
205 btn: QPushButton = self.sender()
206 xmag, ymag = self.BTNS[f"{PREFIX}.{btn.text()}"][-2:]
--> 207 self._move_stage(self._scale(xmag), self._scale(ymag))
xmag = 2
ymag = 0
self = <micromanager_gui._core_widgets._stage_widget.StageWidget object at 0x7f5d33328ee0>
File ~/Documents/oss/micro/napari-micromanager/micromanager_gui/_core_widgets/_stage_widget.py:218, in StageWidget._move_stage(self=<micromanager_gui._core_widgets._stage_widget.StageWidget object>, x=20, y=0)
216 self._mmc.waitForDevice(self._device)
217 if self._snap_on_click:
--> 218 self._mmc.snap()
self = <micromanager_gui._core_widgets._stage_widget.StageWidget object at 0x7f5d33328ee0>
self._mmc = <CMMCorePlus at 0x7f5d3326cd10>
File ~/mambaforge/envs/pymm/lib/python3.10/site-packages/pymmcore_plus/core/_mmcore_plus.py:648, in CMMCorePlus.snap(self=<CMMCorePlus>, fix=True, *args=())
629 def snap(self, *args, fix=True) -> np.ndarray:
630 """
631 snap and return an image.
632
(...)
646 img : np.ndarray
647 """
--> 648 self.snapImage()
self = <CMMCorePlus at 0x7f5d3326cd10>
649 img = self.getImage()
650 self.events.imageSnapped.emit(img)
File ~/mambaforge/envs/pymm/lib/python3.10/site-packages/wrapt/decorators.py:469, in synchronized.<locals>._synchronized(wrapped=<bound method CMMCorePlus.snapImage of <CMMCorePlus>>, instance=<CMMCorePlus>, args=(), kwargs={})
463 @decorator
464 def _synchronized(wrapped, instance, args, kwargs):
465 # Execute the wrapped function while the original supplied
466 # lock is held.
468 with lock:
--> 469 return wrapped(*args, **kwargs)
wrapped = <bound method CMMCorePlus.snapImage of <CMMCorePlus at 0x7f5d3326cd10>>
args = ()
kwargs = {}
File ~/mambaforge/envs/pymm/lib/python3.10/site-packages/pymmcore_plus/core/_mmcore_plus.py:553, in CMMCorePlus.snapImage(self=<CMMCorePlus>)
551 @synchronized(lock)
552 def snapImage(self) -> None:
--> 553 return super().snapImage()
RuntimeError: This operation can not be executed while sequence acquisition is running.
to reproduce that error:
- launch
- start live mode
- trying moving stage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this "snap_after_stage_moves" function but I think we can remove it for now and see how to add it back once the stage, the snap and the live button widgets are merged. Do you agree?
I think there needs to be a UI element showing the difference between a single arrow move vs a double arrow move. As it stands I'm not actually sure of the difference. |
Ahh I see this in the tooltip so less urgent - but still it was not immediately obvious. I think MM behavior of allowing you to set two values is pretty nice. One for fine adjust, and one for something like moving a field of view over |
Codecov Report
@@ Coverage Diff @@
## main #142 +/- ##
==========================================
+ Coverage 80.66% 81.56% +0.90%
==========================================
Files 24 25 +1
Lines 2100 2268 +168
==========================================
+ Hits 1694 1850 +156
- Misses 406 418 +12
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with this whenever you guys are. We can follow up with autofocus stuff in another PR. @fdrgsp, do you feel like this is complete as is? or want more time with it?
(thanks again for taking this over the finish line!!)
You can have look, I think this is complete! However I'm not sure it will work well if there is an autofocus device in the cfg (e.g. Nikon PFS)...these devices in micromanager have 2 "modules", one of which is considered as a Z-Stage device. A stage widget will therefore be generated for these devices as well and I'm not sure if there will be any error (maybe when trying to move the stage through the control buttons...) |
@ianhi I think it does not matter much at the moment. I believe we can discuss about the gui "structure" once all the widgets will be ready (in a widget package). What do you think? |
I've been using this to do experiments and definitely like this much less so it matters to me at least :). This is would be a decrease in usefulness to me and I would probably run a local branch re-integrating it instead of running off main.
Totally agree, but that's also why I think changes like this should not be included in PRs like this one. Improving the widget and changing the UI layout are really orthogonal to each other so lumping them into one PR can make keeping track of things a bit confusing. I'd rather that we should determine the overall plan and implement it wholesale and consistently across all components. |
Thanks for being flexible @fdrgsp - really appreciate it :) I'm now noticing an issue wiht the QCollapsibles - which may be an issue from superqt (so heads up @tlambert03 ) |
yeah, I noticed that too. it is in superqt. and it came from the fix in https://github.com/napari/superqt/pull/72 :/ |
That aside everything looks fine on the demo cam. I will try to try this out a real scope today or tomorrow (pending availability). Should we wait until that's fixed in superqt, or can we forge ahead assuming that superqt will be fixed before it's next release? |
Ok! I tried this out on a real scope and have the following notes:
|
At the moment the order depends on the device name (alphabetical order). But I made the widgets draggable, so you can manually change the position. Do you think we need to do something different?
I will add the poll checkbox also to the z stages:) |
oooh neat. We should try to make that more discoverable. I would never have figured that out if you hadn't told me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good user experience and a nice improvement! Can we merge and the iterate more (autofocus etc) in future PRs?
@ianhi I added a line of code and now we always display the XY stages first and then the Z stages (widgets still draggable). |
🤩 |
creating a new General purpose StageDevice Widget