-
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
feat(api): warn tc-lid/gantry collision in simulation #4394
Conversation
If a given protocol would target the labware loaded into a thermocycler module, while that module is known to be closed, simulation will fail with a helpful message. Closes #4044
Codecov Report
@@ Coverage Diff @@
## edge #4394 +/- ##
==========================================
+ Coverage 56.01% 57.07% +1.06%
==========================================
Files 899 899
Lines 25492 26600 +1108
==========================================
+ Hits 14280 15183 +903
- Misses 11212 11417 +205
Continue to review full report at Codecov.
|
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.
Functionality looks good but I'd love to see a slight reorganization where this checking is the responsibility of the module context.
to_lw, to_well = geometry.split_loc_labware(location) | ||
from_lw, from_well = geometry.split_loc_labware(from_loc) | ||
|
||
if (isinstance(to_lw, Labware) and |
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.
Since module geometries hold refs to their child labware, could we do:
tc = next(m for m in in self._ctx._modules if isinstance(m, ThermocyclerContext))
if tc and tc.labware is to_lw or tc.labware_is from_lw and tc.lid_position=='closed':
raise
And maybe make it a method of the thermocycler context? I think that way move_to
would look a bit cleaner, basically saying "if we have a thermocycler, ask it if this move is OK". and if we add more modules that have motion restrictions (and we certainly could, right - this could be the place we check for those north/south collisions if we ever decide to do that) we'll add similar helpers for other modules without having their implementations padding out move_to
.
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.
code lgtm, haven't tested yet
Also tested locally in simulation |
If a given protocol would target the labware loaded into a thermocycler module, while that module is known to be closed, simulation will fail with a helpful message.
Closes #4044
review requests
Attempt to upload a protocol in which you: