-
Notifications
You must be signed in to change notification settings - Fork 186
feat(api): Support slotFootprintAsChild
and slotFootprintAsParent
locating features
#18769
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
Conversation
… lw dev2 We need to think how to handle this case, but for now, let's just raise an error. Also, regenerate the snapshot so we can compare this diff against diffs that will cause genuine positioning changes.
…Child/Parent locating feature
…ootPrintAsChild/Parent feature
Now that the "asChild" and "asParent" locating feature is fully implemented, it's worth updating the snapshots to compare to baseline
…esting more useful
…d labware v3 We still need this in schema 3, woops. The recent addition of the shim did not account for it.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #18769 +/- ##
===========================================
- Coverage 54.65% 24.57% -30.08%
===========================================
Files 3258 3331 +73
Lines 281774 290103 +8329
Branches 29494 36110 +6616
===========================================
- Hits 153999 71302 -82697
- Misses 127585 218780 +91195
+ Partials 190 21 -169
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
parent_spring_force = parent_deck_item.features["slotFootprintAsParent"].get( | ||
"springDirectionalForce" | ||
) | ||
child_spring_force = child_labware.features["slotFootprintAsChild"].get( |
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.
what does slotFootPrintAsChild.springDirectionalForce
mean? like, "this labware is compatible with a spring parent", where it's true for almost everything? everything?
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.
The labware could be placed on any parent with a slotFootprintAsParent
, and the child's spring determines the child's positioning on the parent. This currently applies to the temp mod calibration plate. If there is a unique or novel interaction, the way we would handle that theoretical scenario is with a new locating feature.
There's a description of this behavior provided in this PR here: shared-data/labware/schemas/3.json
, but happy to update it if you feel it's insufficient. When we get around to the documentation, we'll ensure this is thoroughly explained.
shared-data/labware/definitions/3/schema3test_aluminum_flat_bottom_plate/999.json
Show resolved
Hide resolved
shared-data/labware/definitions/3/schema3test_flex_96_tiprack_200ul/999.json
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/state/_labware_origin_math.py
Outdated
Show resolved
Hide resolved
Ok, turns out that snapshots were indicating that a lot of stackups involving center-to-center mating were in fact moving the well to effectively "D2" instead of "D3" as should happen, and I missed that. The calculation was a bit off, and updating the deck and module definitions to shift the Diffing the snapshot in be7b329 compared to 7db8cfb highlights the changes and makes things look reasonable. The thermocycler cases may be off, but it's hard to tell without an in-office repro, and also, the "asParent" feature won't exist for the TC in a bit, too. The integration tests are now timing out on CI. I'm fixing the root issue soon, so I think it's fine to remove the auto-CI run for now. No one else should be touching the files that impact these tests, anyway. |
4af40c4
to
5bc0eb7
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, thanks! Changes requested for some numbers in the module definitions. Various other things don't need action in this PR.
api/tests/opentrons/protocol_engine/state/test_labware_origin_math.py
Outdated
Show resolved
Hide resolved
"slotFootprintAsChild": { | ||
"slotFootprintAsParent": { | ||
"z": 0, | ||
"backLeft": { | ||
"x": 0, | ||
"y": 0 | ||
"y": 86 | ||
}, | ||
"frontRight": { | ||
"x": 128, | ||
"y": -86 |
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.
@mjhuff I believe what your code does now, and what your intent is for now, is to interpret a module definition's labwareOffset
and slotFootprintAsParent
as combining, right?
So, as a contrived example, to find the back-right corner of the slot atop a module:
- Start with the front-left corner of the underlying deck slot or addressable area beneath the module. (They should be the same thing.)
- Consulting the module definition's
slotTransforms
, apply transforms specific to the deck type and slot. This is the thing that moves the bottom of a Flex module "underground." - Add the module definition's
labwareOffset
. On a Flex, this moves us back up to deck level. - Add the module definition's
slotFootprintAsChild.backLeft.y
andslotFootprintAsChild.frontRight.x
. These take us from the slot origin to the back-right corner.
If that's correct, then I think these numbers are wrong. They would be correct if labwareOffset
were (0, 0, 0), but it's (-0.125, 1.125, 68.275), so it's offsetting the whole slotFootprintAsChild
to the wrong place.
I don't know where the existing labwareOffset
of (-0.125, 1.125, 67.275) comes from. @vegano1 do you know? Could it have been copy-pasted from heaterShakerModuleV1.json
?
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.
The labwareOffset
is almost vestigial at this point and applies to the OT-2 only despite being included for all the Flex modules. The "asParent" numbers often overlap with the bounding box, but not always (see other comment). Happy to chat about this, too.
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.
If what you're describing is correct, then yeah, I agree. The labwareOffset
is incorrect and likely an error; it should be 0, especially for the Flex Stacker, which is not compatible with the OT-2.
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.
api/src/opentrons/protocol_engine/state/_labware_origin_math.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/state/_labware_origin_math.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/state/_labware_origin_math.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/state/_labware_origin_math.py
Outdated
Show resolved
Hide resolved
api/src/opentrons/protocol_engine/state/_labware_origin_math.py
Outdated
Show resolved
Hide resolved
…t for TC/AR incorrect values
Diffing the snapshot in 1aed6ae against 3b49e8a shows no changes, which is what we'd hope to see. The only known wrong values in the snapshot are the z coordinates for the TC and mag block labware stackups. The stacking offsets associated with them will be incorporated when they get their own respective locating features. |
1aed6ae
to
62ee425
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.
LGTM once linting passes. Thank you, this is a huge step!
raise ValueError("Expected parent deck item to have features.") | ||
|
||
|
||
def _module_slot_footprint_as_parent( |
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.
Might be worth leaving a comment on _module_slot_footprint_as_parent
and _aa_slot_footprint_as_parent
describing that they're normalization shims, in place until we delete labwareOffset
+labwareInterfaceXDimension
+labwareInterfaceYDimension
(in module defs) and boundingBox
(in deck defs) and replace them with the same slotFootprintAsParent
shape that exists in labware definitions.
api/src/opentrons/protocol_engine/state/_labware_origin_math.py
Outdated
Show resolved
Hide resolved
"slotFootprintAsChild": { | ||
"slotFootprintAsParent": { | ||
"z": 0, | ||
"backLeft": { | ||
"x": 0, | ||
"y": 0 | ||
"y": 86 | ||
}, | ||
"frontRight": { | ||
"x": 128, | ||
"y": -86 |
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.
Sее EXEC-1648 for the flex stacker module def update. |
d4e2bcd
to
56490f4
Compare
Works toward EXEC-77 and closes EXEC-1487
Overview
Adds support for the
slotFootprintAsChild
andslotFootprintAsParent
locating features as defined in the locating features v2 proposal.Notably, this adds support for parent-child stackups that comprise of:
In addition to the explanation provided in the proposal, it's helpful to view the impact that these new locating features have on our existing labware offset system. To make these snapshots as useful as possible, a couple development shims are added, such as the one in 37728dc. There are two snapshots in the commit history:
The only semi-unexpected side-effect of these changes are in 94d85cf, in which we update protocol validation, since v3 won't need stacking offsets. We will gate labware mating via locating features.
e54f045 is more of a fix than a feat, but pretty self explanatory when viewing the commit. This one slipped through the last PR since there was nothing making use of the definitions in the last PR.
Test Plan and Hands on Testing
Changelog
slotFootprintAsChild
andslotFootprintAsParent
locating features to labware v3.Review requests
Risk assessment
none in the sense the migration hasn't happened yet
low in the sense that we are covered by snapshot testing