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

feat: add minimal Points plan view #316

Merged
merged 48 commits into from
Jul 10, 2024

Conversation

tlambert03
Copy link
Member

@tlambert03 tlambert03 commented Jul 8, 2024

borrowed and modified from #308

hey @fdrgsp, rather than editing each line of the existing FOVSelector, I decided to pull out the bits that I understood and make a bare minimal working widget. I'm sure that there are some features being missed here, but we can add them back as you point them out (lets be relatively strict about features though).

the main thing I did here was just to allow the GraphicsScene to use real-world coordinates. the qgraphics framework provides all sorts of conveniences in mapping between coordinate frames of the view and the scene, so to me it seems to make a lot of sense just to place the scene in the "real world" (using microns, just as useq schema positions will use), and then adjust the view as needed. (pen size is the one annoying thing here, it works as is here, but i think that could also be smarter... need to read more qt docs). I'll try to add bullet points of things that I know i've removed, and we can add them back, but you can also start a bulleted list as well.

would be great if you could play with this and tell me what you see

Screenshot 2024-07-09 at 4 59 28 PM
  • add back black line path showing the order of acquisition
  • discuss the warning that was emitted when the well size didn't match the plate?
  • add back the dotted line that further constrained a random points plan to an area smaller than the well
  • there is a minor bug where if you extend the bounds of the scene, you can't "unextend" them for the purpose of autoresizing. for example, use a grid plan, and add more rows/columns than fit into the coverslip area. The view will (nicely) resize itself to show the full scene... however, if you then reduce the number of rows/cols, it doesn't "recover" and resize back to the usual size
  • deal better with the case of having no _well_width_um

Copy link

codecov bot commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 98.67257% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.36%. Comparing base (a2b18b8) to head (e0336e3).
Report is 30 commits behind head on main.

Files with missing lines Patch % Lines
src/pymmcore_widgets/_util.py 93.33% 1 Missing ⚠️
...useq_widgets/points_plans/_points_plan_selector.py 97.77% 1 Missing ⚠️
...useq_widgets/points_plans/_random_points_widget.py 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #316      +/-   ##
==========================================
+ Coverage   90.07%   90.36%   +0.28%     
==========================================
  Files          71       73       +2     
  Lines        7930     8135     +205     
==========================================
+ Hits         7143     7351     +208     
+ Misses        787      784       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fdrgsp
Copy link
Collaborator

fdrgsp commented Jul 8, 2024

if isinstance(plan, useq.RandomPoints):
with signals_blocked(self.random_points_wdg):
self.random_points_wdg.setValue(plan)
self.random_radio_btn.setChecked(True)
elif isinstance(plan, useq.GridRowsColumns):
with signals_blocked(self.grid_wdg):
self.grid_wdg.setValue(plan)
self.grid_radio_btn.setChecked(True)
elif isinstance(plan, useq.RelativePosition):
with signals_blocked(self.single_pos_wdg):
self.single_pos_wdg.setValue(plan)
self.single_radio_btn.setChecked(True)
else: # pragma: no cover
raise ValueError(f"Invalid plan type: {type(plan)}")

I think at the end here we need to add self.valueChanged.emit(self.value()) otherwise, if the radio button is not changing , the setChecked method won't trigger the update of the WellView...

I believe this will also solve this

there is a minor bug where if you extend the bounds of the scene, you can't "unextend" them for the purpose of autoresizing. for example, use a grid plan, and add more rows/columns than fit into the coverslip area. The view will (nicely) resize itself to show the full scene... however, if you then reduce the number of rows/cols, it doesn't "recover" and resize back to the usual size

no it does not :)

@tlambert03
Copy link
Member Author

tlambert03 commented Jul 8, 2024

added a line , and made it so that if the plan can't generate enough points, it suppresses the usual warning and just caps the widget

Untitled.mov

@tlambert03
Copy link
Member Author

tlambert03 commented Jul 9, 2024

ok, i think this is getting closer. remaining issues above are

  • use random points max_width to further constrain random area, as @fdrgsp had it before
  • deal with scene bounds that ratchet up
  • deal with (or forbid) well_width_um = None
  • add tests

i think the first point can come later, the second is also optional. @fdrgsp, if you want to take a more critical look here at what has been done (knowing we can add more in future PRs) that would be great

@tlambert03 tlambert03 changed the title minimal Points plan view feat: add minimal Points plan view Jul 9, 2024
@tlambert03
Copy link
Member Author

ok, I think all the things I wanted to deal with (besides tests) are now dealt with. Take another go at using it, and let me know how it feels. then I'll bring over the tests you wrote for this part, and fix them to work with any changes

@tlambert03 tlambert03 marked this pull request as ready for review July 10, 2024 15:02
@tlambert03
Copy link
Member Author

ok, tests added. Assuming they pass, feel free to merge this when ready @fdrgsp, and add the further improvements in a followup

@fdrgsp fdrgsp merged commit 370dfcd into pymmcore-plus:main Jul 10, 2024
20 checks passed
@tlambert03 tlambert03 added the enhancement New feature or request label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants