-
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
refactor(api): Save full nozzle map configuration and update state store accordingly #14529
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14529 +/- ##
=======================================
Coverage 67.78% 67.78%
=======================================
Files 2517 2517
Lines 72043 72043
Branches 9274 9274
=======================================
Hits 48837 48837
Misses 20991 20991
Partials 2215 2215
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 looks good as long as it has the right serializability - will this get persisted?
self._state.nozzle_configuration_by_id[ | ||
private_result.pipette_id | ||
] = config.nozzle_map |
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 make nozzle_configuration_by_id
values non-Optional
now?
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.
Discussed with @Laura-Danielle. We'll defer this change. Might want to leave a # TODO
explaining what needs to happen, though.
I think we're good. I'm not seeing any modification of Pydantic |
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.
Sweet. Thanks!
We won't have a chance to test this on any robots for a bit. Your call whether you want to merge this before then. I'm not sure how much other stuff is blocked behind this.
self._state.nozzle_configuration_by_id[ | ||
private_result.pipette_id | ||
] = config.nozzle_map |
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.
Discussed with @Laura-Danielle. We'll defer this change. Might want to leave a # TODO
explaining what needs to happen, though.
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! Just the comment about better test assertions
tip_configuration_lookup_table=result1.tip_configuration_lookup_table, | ||
nominal_tip_overlap=result1.nominal_tip_overlap, | ||
back_left_nozzle_offset=Point(x=-8.0, y=-22.0, z=-259.15), | ||
front_right_nozzle_offset=Point(x=-8.0, y=-22.0, z=-259.15), | ||
nozzle_map=result1.nozzle_map, |
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.
Hmm, just realizing these aren't great asserts. They're just comparing an item with itself; like result1.nozzle_map
== result1.nozzle_map
. We should check it against the actual expected nozzle map.
@@ -105,8 +100,7 @@ def test_configure_virtual_pipette_for_volume( | |||
), | |||
tip_configuration_lookup_table=result2.tip_configuration_lookup_table, | |||
nominal_tip_overlap=result2.nominal_tip_overlap, | |||
back_left_nozzle_offset=Point(x=-8.0, y=-22.0, z=-259.15), | |||
front_right_nozzle_offset=Point(x=-8.0, y=-22.0, z=-259.15), | |||
nozzle_map=result2.nozzle_map, |
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.
same as above
`nozzle_map` cannot be `None` since #14529.
`nozzle_map` cannot be `None` since #14529.
Overview
To make a few operations easier in protocol engine, we will keep the nozzle map in state always. I will think about this further in RSS-443, but for now it should unblock other partial tip work.
Test Plan
General sanity check that partial tip still works and regular protocols operate as normal.
Changelog
LoadedStaticPipetteData
Review requests
Check that I didn't miss anything to update.
Risk assessment
Low/medium. Currently not using this information anywhere aside from back left/right values which will remain the same. Tests only updated due to new helper function pulling in mock data.