(mx-bluesky#873) Factor out drawing of crosshair from snapshot device#1095
(mx-bluesky#873) Factor out drawing of crosshair from snapshot device#1095
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1095 +/- ##
=======================================
Coverage 97.75% 97.75%
=======================================
Files 170 171 +1
Lines 6851 6853 +2
=======================================
+ Hits 6697 6699 +2
Misses 154 154 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Thanks, couple of minor bits
| def compute_beam_centre_pixel_xy_for_mm_position( | ||
| sample_pos_mm: Sequence[float], | ||
| beam_pos_at_origin_px: Sequence[int], | ||
| microns_per_pixel: Sequence[float], | ||
| ) -> Sequence[int]: |
There was a problem hiding this comment.
Should: I think this needs a docstring and better typing. It's not obvious how long the sequences of floats/ints should be so I think it would be better to use tuple[float, float]. You could even reuse Pixel as defined in oav/utils. In fact, it's only through reading how this is used in mx-bluesky that I've realised that sample_pos_mm is not the absolute sample position but rather the sample position relative to what it was when the snapshot was taken, correct?
There was a problem hiding this comment.
Could: Most of the maths for trying to translate beam position, smaple position is currently in the utils file, maybe this makes more sense there?
There was a problem hiding this comment.
Should: I think this needs a docstring and better typing. It's not obvious how long the sequences of floats/ints should be so I think it would be better to use
tuple[float, float]. You could even reusePixelas defined inoav/utils. In fact, it's only through reading how this is used inmx-blueskythat I've realised thatsample_pos_mmis not the absolute sample position but rather the sample position relative to what it was when the snapshot was taken, correct?
I started out with tuple[int, int] etc. However type checking then requires # type: ignore because the for comprehension is determined to be of type tuple[int, ...] instead of tuple[int, int]
Your assumption about the sample_pos_mm is correct. I originally started writing this before the GH ticket split, so there are some things that anticipate more information being captured from the grid snapshots, which possibly I could have tidied up more.
There was a problem hiding this comment.
I have changed tuple[int, int] to Pixel and it makes the type-checking issue go away. I have no idea why since Pixel = tuple[int, int]. Damn you python type annotations!! <shakes fist at cloud/>
| ) | ||
|
|
||
|
|
||
| class SnapshotWithBeamCentre(MJPG): |
There was a problem hiding this comment.
Should: This kind of feels redundant now? Certainly if it's just taking the snapshot then it doesn't need any reference to beam centre in either the name or holding the references?
There was a problem hiding this comment.
Could: We might be able to just fold the saving image on trigger into the base MJPG class as it's probably common thing you want the camera to do
rtuck99
force-pushed
the
mx-bluesky-873_move_crosshair_logic_to_callback
branch
2 times, most recently
from
936878b to
5b9b380
Compare
* Rename and simplify SnapshotWithBeamCentre to Snapshot * Change compute_beam_centre signature
Change tuple[int, int] to Pixel
rtuck99
force-pushed
the
mx-bluesky-873_move_crosshair_logic_to_callback
branch
from
5b9b380 to
08b6033
Compare
2 participants
Required for
See main PR
Checks for reviewer
dodal connect ${BEAMLINE}