feat(api,robot-server): check pipette offset consistency #13583
Conversation
Add a basic instrument offset consistency check to drive UI indicating that an insturment may be miscalibrated and the user should run calibration. Closes RAUT-738
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.0.0 #13583 +/- ##
====================================================
Coverage 71.32% 71.32%
====================================================
Files 2422 2422
Lines 68142 68142
Branches 7934 7934
====================================================
Hits 48605 48605
Misses 17680 17680
Partials 1857 1857
Flags with carried forward coverage won't be shown. Click here to find out more.
|
sfoster1
changed the title
|
|
||
| @dataclass | ||
| class InconsistentPipetteOffsets: | ||
| kind: Literal["inconsistent-pipette-offset"] |
There was a problem hiding this comment.
why do we need kind if InconsistentPipetteOffsets is already its own class?
There was a problem hiding this comment.
It's to have future support for the thing that goes in the reasonability_check_failures being a discriminated union if we add more checks
There was a problem hiding this comment.
but we're already calling the dataclass InconsistentPipetteOffsets which feels restricted to me. Also what if the other checks require other data inputs that don't map to what InconsistentPipetteOffsets requires? I'm just not sure if we need to worry about future proofing that much at the moment.
There was a problem hiding this comment.
Right, this dataclass is called InconsistentPipetteOffsets, but if you look below, the type that's bound into the PipetteOffsetSummary is actually ReasonabilityCheckFailure which is a union of (right now) one element. And having other checks have different data is exactly what the discriminated union is for - we'd definitely have to change stuff internally but we wouldn't have to change the shape of the server code or responses.
| ): | ||
| return [] | ||
| diff = left_offset - right_offset | ||
| if any(abs(d) > PIPETTE_OFFSET_CONSISTENCY_LIMIT for d in diff): |
There was a problem hiding this comment.
Where does the limit come from?
There was a problem hiding this comment.
discussions with hardware
| return [ | ||
| InconsistentPipetteOffsets( | ||
| "inconsistent-pipette-offset", | ||
| {Mount.LEFT: left_offset, Mount.RIGHT: right_offset}, |
There was a problem hiding this comment.
Don't we already provide the offsets in the UI for users, could we not just pass in the difference and/or only show th e limit here?
There was a problem hiding this comment.
yeah, but it feels kind of nice to have it in the error class (kind of like the details fields in the error codes)
| taken as an advisory. | ||
| """ | ||
|
|
||
| kind: Literal["inconsistent-pipette-offset"] = "inconsistent-pipette-offset" |
There was a problem hiding this comment.
Same comments for this as per the dataclass in instruments/ot3
There was a problem hiding this comment.
same answer - adds support for a discriminated union of different kinds of checks with different payloads
Add a basic instrument offset consistency check to drive UI indicating that an instrument may be miscalibrated and the user should run calibration.
This checks the consistency between the calibrated pipette offsets of the left and right pipettes. It detects a problem if
In other situations they succeed.
The outcome of this check shouldn't be treated as definitely meaning a calibration is bad; rather, it can serve as a hint that a user might want to retry calibration (so for instance if we display a banner on this it should be dismissable).
Closes RAUT-738
Testing