-
Notifications
You must be signed in to change notification settings - Fork 179
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
fix(api): restrict the labware that can be moved to the plate reader + validate wavelengths. #16649
fix(api): restrict the labware that can be moved to the plate reader + validate wavelengths. #16649
Conversation
f32ce9d
to
4c2831e
Compare
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.
Looks great, should do what we need. Just a question below about clarifying our final pick for the max height restriction.
def raise_if_labware_incompatible_with_plate_reader( | ||
self, | ||
labware_id: str, | ||
) -> None: | ||
"""Raise an error if the labware is not compatible with the plate reader.""" |
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.
Solid, the two cases in here should cover us pretty well.
@@ -81,6 +81,10 @@ | |||
} | |||
|
|||
|
|||
# The max height of the labware that can fit in a plate reader | |||
_PLATE_READER_MAX_LABWARE_Z_MM = 15 |
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.
Are we sure we want the limit to be 15mm? Inner dimension of the plate reader lid was measured to approximately 16mm.
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.
yeah, just as a margin of error.
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 would rule out the Tough PCR plate, which has a definition height of 16.00 mm. I think we definitely want to allow it.
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.
@ecormany
good call out, fixed it.
I'm getting simulation/analysis errors just trying to load a plate reader on this branch. Is this something wrong with my setup? Protocol (truncated, but it fails on line 23, the one that starts
Result
|
Yeah there was a bug, should be fixed now. |
369c920
to
e25402a
Compare
5b86acb
to
6fc53da
Compare
It loads and the restrictions on wavelengths are working now too! Fantastic. One question: do we care to distinguish between loading and moving oversize labware to the reader? Currently they both raise the same error:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## chore_release-8.2.0 #16649 +/- ##
======================================================
Coverage ? 92.43%
======================================================
Files ? 77
Lines ? 1283
Branches ? 0
======================================================
Hits ? 1186
Misses ? 97
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. |
04b0cc8
to
a950f15
Compare
validate wavelengths for analysis update max height to 16mm and clean up
a950f15
to
3129f05
Compare
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.
Looks good but maybe make the wavelength limits a constant?
) | ||
|
||
if reference_wavelength is not None and ( | ||
reference_wavelength < 350 or reference_wavelength > 1000 |
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.
can we maybe get these as consts
Overview
This pull request restricts labware that can be loaded or moved onto a plate reader slot
This also restricts the wavelengths that can be used
Closes: PLAT-541 PLAT-581
Test Plan and Hands on Testing
LabwareMovementNotAllowedError
if the labware is not 96 wells or the height is > 15mmChangelog
raise_if_labware_incompatible_with_plate_reader
that checks labware compatibility with the plate readerraise_if_labware_incompatible_with_plate_reader
inLoadModule
andMoveLabware
commands.Review requests
raise_if_labware_incompatible_with_plate_reader
Risk assessment
low