feat(api): Partial tip pickup support for Single, Row and Partial Column #15585
Conversation
…ozzle to nozzle configuration
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #15585 +/- ##
===========================================
+ Coverage 60.12% 81.03% +20.90%
===========================================
Files 190 118 -72
Lines 10627 4112 -6515
===========================================
- Hits 6390 3332 -3058
+ Misses 4237 780 -3457
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/ |
|
Once this merges, I'll follow up in a separate PR to describe the new parameters in the docstring, as well as describe their function in the corresponding article. |
There was a problem hiding this comment.
Have to go over the tip handler changes but here's some initial thoughts. I think the introduction of end can cause more confusion if it's not handled correctly.
Also, can we break complex functions into smaller contained ones instead of adding noqa tags? noqa tagged functions tend to keep getting messier and harder to read or fix since any additions to it just keep getting ignored.
| if ( | ||
| self._api_version | ||
| < _PARTIAL_NOZZLE_CONFIGURATION_SINGLE_ROW_PARTIAL_COLUMN_ADDED_IN | ||
| ): | ||
| if style not in original_enabled_layouts: | ||
| raise ValueError( | ||
| f"Nozzle layout configuration of style {style.value} is unsupported in API Versions lower than {_PARTIAL_NOZZLE_CONFIGURATION_SINGLE_ROW_PARTIAL_COLUMN_ADDED_IN}." | ||
| ) |
There was a problem hiding this comment.
Can be merged into one if statement
| primary_nozzle=start, | ||
| front_right_nozzle=front_right, | ||
| back_left_nozzle=back_left, | ||
| ) | ||
| # TODO (spp, 2023-12-05): verify that tipracks are on adapters for only full 96 channel config |
There was a problem hiding this comment.
Can remove this todo. We're doing that check durin pipette movement/ deck conflict check.
| assert primary_nozzle is not None | ||
| assert front_right_nozzle is not None or back_left_nozzle is not None |
There was a problem hiding this comment.
These should be verified in the configure_nozzle_layout API with user-friendly error messages rather than asserting here. (Looks like 'primary nozzle is not None' is already asserted in the API so this one is redundant).
With the verification done in the API method, this implementation can just use QuadrantNozzleLayoutConfiguration with the available args without verifying for partial column or plain old quadrant separately as done here and on lines 826-832.
There was a problem hiding this comment.
I think the introduction of end nozzle is adding further confusion and redundancy in this code. What if someone specifies the end nozzle instead of back_left or front_right and vice versa? If the start and end combo makes more sense to protocol authors, then maybe we should think about removing back_left and front_right from the API and keep it only in the layers underneath. Has this been discussed? Are there any reasons not to remove them? It will be a breaking change but a very low stakes since the released configurations until now only used the start nozzle. We could also deprecate them slowly so that it's not a breaking change.
If there are compelling reasons to not remove the redundant args, we should check that something like this should fail expectedly-
context.configure_nozzle_layout(style=PARTIAL_COLUMN, start="A1", end="D1", back_left="B1" )
There was a problem hiding this comment.
Theoretically you would want to keep all three, because if in the future quadrants exist as a concept then there are some cases where a user will need to specify a primary, front right and back left nozzle. One such situation would be making a 4x4 tip Quadrant with A12 as the primary nozzle (start), A9 as the back_left nozzle, and D12 as the front_right nozzle. Such a configuration would be impossible to create without all three parameters. Primary nozzle. H1 as primary has a similar potential issue. The addition of end as a parameter exists to simply the usage of Partial columns specifically, with end acting as a reflexive parameter (it is either the back left or front right, depending on the position of start). I do not believe it over complicates things, so long as we have good documentation to explain the addition of end. This was something @ecormany and I discussed regarding the documentation and functionality of the configure_nozzle_layout command.
Regardless, if we are going to keep it I will ensure that the error case you specified results in expected failure.
There was a problem hiding this comment.
@CaseyBatten Sorry, basic question. In that three-parameter situation you're describing, what's the special significance of the "primary" nozzle being A9? Why would someone want that instead of the primary nozzle being A9 or D12? What does that actually do?
Is it for choosing the direction in which tips are consumed, or is it for choosing which tip moves to the well when you do pipette.move_to(single_well), or both, or something else?
I don't understand this well enough to have an opinion on what to do, but my knee-jerk reaction is that all of this is quite confusing and we should try to simplify it if we can. As a user, style, start/end, and front_right/back_left seem like three different competing ways of specifying the same-ish thing?
A process note: I think I'm finding this confusing because I'm not seeing a complete, final summary of how the new interface is supposed to work. For example, I would have found it helpful if this PR updated the InstrumentContext.configure_nozzle_layout() docstring, or if there were a draft of it in Confluence. I see there's some existing discussion in PLAT-51, but it looks like a bit to catch up on; and you've talked about the new param in the PR description, but I'm not familiar enough with this feature for that to really make sense to me.
There was a problem hiding this comment.
My bad for encouraging not updating the docstring here — there was some thought that we'd try to keep that out of edge and in a feature docs branch, but we already have lots of PAPI 2.20 content in edge and shouldn't feel restrained from adding more.
There was a problem hiding this comment.
@SyntaxColoring Sorry let me clarify, as I think the formatting of that paragraph was confusing. The situation i was discussing was the following:
- start (primary nozzle) = A12
- back_left = A9
- front_right = D12
This will result in a 4x4 tip pickup in our "Quadrant" configuration (currently an engine-only configuration)
What this will look like is this on the pipette:
A9 X X A12
X X X X
X X X X
X X X D12
So in this example, the three separate values define the edges of our tip "box" in the corner of the pipette. This is the only way to define a quadrant in corner A12, and also the only way to define a quadrant in corner H1 for the same reason.
@ecormany I agree it, I'll go through and add in the doc string explanations in this PR. It wasn't a bad idea to try to unify those elsewhere, but it played out against our favor here.
There was a problem hiding this comment.
To further explain that example and address your questions Max, we're defining which nozzles on the pipette will be physically used, which will effect which tips are consumed and in what order on the tiprack. In short, if you Pick H12 as the primary nozzle on a 96ch, you will consume tips in our traditional fashion (top left to bottom right or tiprack). If you pick A1 as the primary nozzle on a 96ch, the opposite will occur (bottom right to top left). In essence, whichever starting nozzle you choose, the opposite welled tip will be selected on the tiprack for pickup. For the 8ch, this only applies on a per column level, so selecting H1 as your primary nozzle will pick up tip A1 on the tiprack.
The reasoning for the cross-behavior there is because of the physical layout of our pipettes. As an example, with an 8ch pipette if you tried to pick up tip H1 with nozzle H1, you would end up picking up all the tips from H1-A1 because the other 7 nozzles would line up with the other 7 tips. Same logic applies for the 96ch but on two axes.
There was a problem hiding this comment.
Thanks so much for that explanation and the updated docstring. I've read those and PLAT-51 and it's all making a lot more sense to me now. I have more suggestions and questions, but we'll move that to Slack.
|
A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/ |
|
A PR has been opened to address analyses snapshot changes. Please review the changes here: https://github.com/Opentrons/opentrons/pull/ |
|
Docstring additions look good to me, although I suspect that the note will eventually get folded into one of the API articles. |
Overview
Covers PLAT-51, PLAT-217 and PLAT-355
Publicly exposes Single and Row configurations. Introduces Partial Column configuration and the necessary logic to support it. Expands quadrants to accept a back_left_nozzle qualifier to help define shapes from the reverse direction, and partial columns build upon that infrastructure.
Test Plan
Changelog
Review requests
The addition of
end: Optional[str] = None,toconfigure_nozzle_layoutwas a decision based on input regarding what felt natural to a user when utilizing the Partial Column command. Functionally,endis either theback_left_nozzleorfront_right_nozzledepending on the position of thestartor primary nozzle. In this way, it is technically repetitious to haveendas a parameter at all, but it may be beneficial from a readability and usability standpoint when creating linear partial column configurations (and in the future, partial row configurations). Let me know thoughts on this, if we feel it is unnecessary or if it is something we want to keep.Risk assessment
Medium risk - No major changes to the existing behavior, however introduction of new configurations may mean we need to revalidate our collision checking to ensure that the risk of putting the pipette into a dangerous situation or commanding it to move outside of the robots extents cannot occur.