Pin tip centring now takes a median of values #242
Conversation
| def statistics_of_positions(positions: List[Tuple]): | ||
| x_coords, y_coords = np.array(positions).T | ||
|
|
||
| median = (int(np.median(x_coords)), int(np.median(y_coords))) |
There was a problem hiding this comment.
Why do we want these to be integers?
There was a problem hiding this comment.
Good question. They're pixel values so we assumed they were integers later on for doing things like drawing grids and putting values into ispyb. Maybe I should do this conversion at the point we definitely need integers though
There was a problem hiding this comment.
Actually, there's so many places this could potentially break that I think conversion here makes sense, I will add a comment though.
There was a problem hiding this comment.
So I added type hinting, which makes it clearer that these things are valid Pixel locations and so must be ints. Doesn't really answer your question 100% but hopefully makes it clearer?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #242 +/- ##
==========================================
+ Coverage 88.09% 88.29% +0.19%
==========================================
Files 72 72
Lines 2587 2605 +18
==========================================
+ Hits 2279 2300 +21
+ Misses 308 305 -3 ☔ View full report in Codecov by Sentry. |
| @@ -19,16 +34,37 @@ class PinTipDetect(Device): | |||
|
|
|||
| triggered_tip: Signal = Component(Signal, kind=Kind.hinted, value=INVALID_POSITION) | |||
| validity_timeout: Signal = Component(Signal, value=5) | |||
| settle_time_s: Signal = Component(Signal, value=0.5) | |||
|
|
|||
| tip_positions: List[Tuple] = [] | |||
There was a problem hiding this comment.
It looks like we don't need to define tip_positions here if it's also defined in trigger()
There was a problem hiding this comment.
This looks good, I think the tests are good too. I was going to comment on doing some tests regarding the logic of StableSubscriptionStatus, eg test_trigger_status_timeout_if_pin_doesnt_stay_valid, but I guess since this is a tested function in Ophyd then there's no need
Hot fixes part 2 of DiamondLightSource/hyperion#996
Instructions to reviewer on how to test:
Checks for reviewer