chore: Separate lt and ht pickup and dropoff #13962

sfoster1 · 2023-11-09T21:26:46Z

This PR looks gigantic but most of it is little fiddly fixups to pipette config files that are schema checked and don't change data.

Our 96 channel pipettes pick up full racks of tips using a cam grabber, but they're also capable of picking up less tips than that through the normal press-fit routine that the low throughput pipettes use. Those routines need different configuration values that do not overlap, and we previously had them overlapping.

This commit

Changes the pipette config general schema (this is a good place to start when reviewing) to fully separate the two styles of pickup (press fit and cam action) and the two styles of drop tip (plunger eject and cam action). Low throughput pipettes do press fit and plunger eject; high throughput pipettes do both kinds of pickup, and cam action drop. Current configuration is folded in to the two new locations.
Changes all the other code to adapt to that
Changes the pipette geometry schema to add the rows and columns in which the nozzles are arranged so we can do partial tip pickup calculations based on it
Changes the partial tip pickup stuff to work based on symbolic geometry instead of points

Review requests

Check that no data changed except in a9ec639
Do the new configs make a little more sense

Testing

Pick up some tips!

codecov · 2023-11-09T21:28:40Z

Codecov Report

Merging #13962 (a0293f0) into edge (b012533) will increase coverage by 0.02%.
The diff coverage is 80.15%.

Additional details and impacted files

@@            Coverage Diff             @@
##             edge   #13962      +/-   ##
==========================================
+ Coverage   70.66%   70.68%   +0.02%     
==========================================
  Files        2505     1627     -878     
  Lines       70819    54225   -16594     
  Branches     8743     3849    -4894     
==========================================
- Hits        50043    38331   -11712     
+ Misses      18640    15232    -3408     
+ Partials     2136      662    -1474

Flag	Coverage Δ
app	`38.62% <ø> (-29.15%)`	⬇️
components	`62.79% <ø> (ø)`
g-code-testing	`96.44% <ø> (ø)`
hardware-testing	`∅ <ø> (∅)`
labware-library	`51.50% <ø> (ø)`
notify-server	`89.13% <ø> (ø)`
protocol-designer	`45.67% <ø> (ø)`
react-api-client	`65.87% <ø> (ø)`
shared-data	`73.58% <80.15%> (+0.42%)`	⬆️
step-generation	`86.57% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
api/src/opentrons/hardware_control/ot3api.py	`79.41% <ø> (ø)`
...obot-server/robot_server/robot/calibration/util.py	`100.00% <ø> (ø)`
...n/opentrons_shared_data/pipette/model_constants.py	`100.00% <100.00%> (ø)`
...pentrons_shared_data/pipette/pipette_definition.py	`94.21% <95.74%> (+0.13%)`	⬆️
.../python/opentrons_shared_data/pipette/load_data.py	`90.90% <86.36%> (+8.08%)`	⬆️
...rons_shared_data/pipette/mutable_configurations.py	`86.26% <81.39%> (-0.86%)`	⬇️
...s_shared_data/pipette/scripts/build_json_script.py	`0.00% <0.00%> (ø)`

... and 878 files with indirect coverage changes

Laura-Danielle

Code in general looks good, just had a comment about some updated field descriptions.

I think also we'll want to do some extra testing on mutable configurations since there are a lot of changes there.

Laura-Danielle · 2023-11-13T15:35:31Z

shared-data/python/opentrons_shared_data/pipette/pipette_definition.py

    presses: int = Field(
-        default=0.0, description="The number of tries required to force pick up a tip."


Can we keep the previous descriptions for these fields? They make more sense imo.

That's not what that is, though. You're going incrementally deeper every time, it's not just a retry

Added suggestions below.

ahiuchingau

THIS IS FANTASTIC! THANK YOU!!

What does cam stand for?

sanni-t

Changes make sense to me. Just a couple of questions.
Sounds like this will need thorough testing of all pipettes? (even OT2) Let me know if you or @caila-marashaj need any help with that. The 96 channel partial tip protocols we've tested so far are here

shared-data/pipette/schemas/2/pipettePropertiesSchema.json

sanni-t · 2023-11-13T16:19:51Z

shared-data/pipette/definitions/2/general/ninety_six_channel/p1000/3_5.json

-      "48": 0.75,
-      "96": 1.5
-    }
+    "availableConfigurations": [1, 8, 12, 16, 24, 48, 96]


Interesting, why are the other versions limited to [1, 8, 12, 96] configurations while this one has all available? Is there something in the old versions that limits their use of other partial configurations?

Human error 😆

@Laura-Danielle Human error like "3.5 should also only have [1, 8, 12, 96] available" or human error like "all versions should have 24 and 48"?

Human error as in I forgot about 16, 24, 48 as tip options. To be fair though, we don't really use this config as much as I thought we would.

will add them

Laura-Danielle

Will not request changes as to ensure I don't block this PR from being merged. However, I do have a few comments that I think we should address before merging.

Laura-Danielle · 2023-11-14T20:07:22Z

api/src/opentrons/hardware_control/nozzle_manager.py

+                if len(current_cols) == len(physical_cols) // 2:
+                    return NozzleConfigurationType.QUADRANT
+
+        return NozzleConfigurationType.SUBRECT


Can we just remove QUADRANT in favor of SUBRECT since a quadrant is a a subrectangle anyways? I can't see a need to distinguish between the two.

We allow actual quadrants but not arbitrary subrects though, right? I feel like it's worth being able to distinguish for that reason

No I've been referring to quadrant as representing arbitrary sub-rectangles. We're never actually verifying (until here I guess) that it is truly a quadrant, but I don't think we really need to do that. I'm currently working on boundary box checks again -- finally -- and I can't see any need to distinguish between the two.

Laura-Danielle · 2023-11-14T20:10:18Z

shared-data/python/opentrons_shared_data/pipette/pipette_definition.py

@@ -359,6 +359,16 @@ class Config:
        }


+class PipetteRowDefinition(BaseModel):


Can we just call this PipetteRowColumnDefinition or something to that effect and get rid of the separate definitions for the two? They have the exact same fields.

They have the same fields but those fields do not have the same types in the json, and the only reason they have the same type here is because our version of pydantic isn't capable of doing fine enough divisions. I actually wrote a whole validator for this before realizing I did it based on future pydantic docs that we can't use, and as soon as we update I'll be adding them again. Specifically, the row names and column names I'd like to be separate and verified independently.

I mean they have the same basic type with some pattern validation in JSON. If you want to split them off here already, then I'd add the validation checks in these pydantic models.

They do not have the same pattern validation. The column names look for /[0-9]+/ and the row names look for /[A-Z]+/. I'll add the validation

I think we should bias to the side of keeping more complex constraints in the jsonschema, though. It's just so much better at it.

shared-data/python/opentrons_shared_data/pipette/pipette_definition.py

Laura-Danielle · 2023-11-14T20:21:40Z

api/src/opentrons/hardware_control/nozzle_manager.py

-        difference = self.map_store[self.back_left] - self.map_store[self.front_right]
-        y_rows_length = int(difference[1] // INTERNOZZLE_SPACING)
-        return map_store_list[starting_idx + y_rows_length]
+        front_left = next(iter(self.columns.values()))[-1]


This technically is only the 'front left' of the 8 channel. It would give an incorrect value if the 96 channel was in any configuration that did not include the leftmost nozzle. Should maybe just determine this in the build function and save.

So actually, and I think that maybe I should clarify this more, in an individual NozzleMap instance the map_store, rows, and columns actually only refer to the tips that are valid in the map. The first element in columns will always be the left-most column of this specific partial tip configuration, therefore the last element of that first column will always be the frontest, leftest channel. This is verified in the test_*_pipette_map_entries tests in test_nozzle_manager.

yeah the frontest/leftest channel of that configuration, not of the whole pipette -- whereas previously it would only ever refer to the front left-most? (H1) of the pipette.

I can't think of an exact use-case where it could be a problem to change that behavior, but something to look out for/be aware of in case there is weird behavior somewhere. I would also change the comment inside the property to be a little more explicit about what's happening.

Sure. I think the idea is that if you want the front-left most of the pipette you would get it from the physical nozzlemap instance that the manager keeps around.

Laura-Danielle · 2023-11-14T20:48:45Z

api/src/opentrons/hardware_control/nozzle_manager.py

+                wrapping=[PythonException(e)],
+            ) from e
+
+        def _rows_in_map() -> Iterator[Tuple[str, List[str]]]:


imo I think this would be cleaner if we just took the first/last index from front/back row/column and then used list splicing.

less lines of code too :D

Yes, but that would require (if I'm understanding right) parsing the row and column from the name of the well, which is something I tried to avoid as much as I could in favor of keeping the well names opaque atomic artifacts, which is an approach that feels more robust. For instance, if we wanted to, there would be nothing preventing us from naming a tip location just 1 or for that matter steve in this implementation - they're fully arbitrary.

The iteration process also keeps a robustness element of handling arbitrarily and possibly irregular pipettes - because we're iterating linearly, we don't actually care how many things we emit, and that's another thing that isn't really necessary but that I like a lot about this.

No you shouldn't need to.

You could just return the back_left_row/back_left_column as a tuple of (name, contents) rather than just name. Then you could do:

list(ordered_dict.items()).index((name, contents))

As long as that matches the internals then you will get the correct index. I think this should also take care of irregular pipette patterns, unless I don't understand what types of irregularities you have in mind.

Oh ok I get it. Yeah I can do that. It won't support irregular format pipettes, like if rows had different number of columns, but we won't have those anyway probably.

@Laura-Danielle how does 39fe151 look

shared-data/python/opentrons_shared_data/pipette/pipette_definition.py

Laura-Danielle · 2023-11-14T20:55:50Z

shared-data/python/opentrons_shared_data/pipette/pipette_definition.py

    presses: int = Field(
-        default=0.0, description="The number of tries required to force pick up a tip."


Added suggestions below.

ryanthecoder · 2023-11-15T16:44:56Z

shared-data/pipette/definitions/2/general/eight_channel/p1000/3_5.json

+      "distance": 13.0,
+      "currentByTipCount": {
+        "1": 0.2,
+        "2": 0.14,


Is this one meant to be lower than everything else

Yeah, these are sort of known-to-be-messed-up first pass numbers

Laura-Danielle

nice, lgtm!

These configs had a couple things to change: - pickup and drop actually represent configuration for two different actions: the press fit / plunger eject sequence that the low throughput pipettes do, and the cam action sequence that the high throughput pipettes do when handling a full rack. information was actually overloaded and lost: there was only room for one speed for both separate actions. By separating the configs for each action, we get that information back - fold in the per tip current information into the pickup config so it looks a bit more natural and also can be distributed between the action kinds

Now that we have separate configuration elements for picking up tips with press fit vs cam action and dropping them with plunger eject and cam action, we need to use them in a way that is nicely integrated with the nozzle configuration stuff. That means that the plan methods should check the current nozzle configuration and use the appropriate config and behavior.

When we're calibrating an OT-2 multi channel, we do it with just one tip. This means we have to drop the current to have the tip attached with the appropriate amount of force. We now have a bunch of other code focused on doing exactly that tied up with the partial tip pickup mechanics, so use it instead.

These are good for 1, 8, and 96 but probably not anything else.

We had implemented partial tip configurations by doing math on the offsets of all the nozzles. This works, but has two downsides: 1. It's sort of hard to read unless you have a good mental model of the physical layouts 2. It relies on doing exact numerical comparisons In general, I think it is never a good idea to try and go _from_ numbers _to_ symbolic knowledge. It's too vulnerable to small perturbations, and it's too annoying to read. Instead, let's encode the structure of the pipette nozzles in the pipette geometry defs by writing down the rows and columns of the pipette layout. Then, we can do our partial configuration logic based on the rows and columns of the pipette. We now do not hit a bug when we request a partial configuration that doesn't include A1; and we now do not rely on array stride math; and we now technically support arbitrary axis-aligned rectangles (at least on this layer - definitely verboten above).

…ition.py Co-authored-by: Laura Cox <[email protected]>

sfoster1 requested review from a team as code owners November 9, 2023 21:26

sfoster1 requested review from mjhuff, sanni-t and Laura-Danielle and removed request for a team November 9, 2023 21:26

sfoster1 force-pushed the separate-lt-and-ht-pickup-and-dropoff branch from febb4ab to 1a525f3 Compare November 9, 2023 22:06

Laura-Danielle approved these changes Nov 13, 2023

View reviewed changes

ahiuchingau approved these changes Nov 13, 2023

View reviewed changes

sanni-t approved these changes Nov 13, 2023

View reviewed changes

sfoster1 requested review from Laura-Danielle and ahiuchingau November 13, 2023 22:35

Laura-Danielle reviewed Nov 14, 2023

View reviewed changes

ryanthecoder reviewed Nov 15, 2023

View reviewed changes

Laura-Danielle approved these changes Nov 15, 2023

View reviewed changes

sfoster1 and others added 13 commits November 15, 2023 17:44

chore(shared-data): 96: correcter partial currents

da7508e

These are good for 1, 8, and 96 but probably not anything else.

3am going back for more schema refactor

0b6e75a

update hardware-testing

b2476e6

js lint

33f7265

fix some python lint issues

e6ce59a

Update shared-data/python/opentrons_shared_data/pipette/pipette_defin…

f35e0e6

…ition.py Co-authored-by: Laura Cox <[email protected]>

Update shared-data/python/opentrons_shared_data/pipette/pipette_defin…

2213b24

…ition.py Co-authored-by: Laura Cox <[email protected]>

Update shared-data/python/opentrons_shared_data/pipette/pipette_defin…

e973136

…ition.py Co-authored-by: Laura Cox <[email protected]>

Update shared-data/python/opentrons_shared_data/pipette/pipette_defin…

1b6de02

…ition.py Co-authored-by: Laura Cox <[email protected]>

sfoster1 added 6 commits November 15, 2023 17:45

format after changes

dd28e28

add pydantic validators for row and col names

71b696f

update docstrings for nozzle maps

bfbeb31

get rid of QUADRANT

562ecac

use slices instead of iteration for style reasons

e788a45

add missing partial configs

a0293f0

sfoster1 force-pushed the separate-lt-and-ht-pickup-and-dropoff branch from ee0636d to a0293f0 Compare November 15, 2023 22:45

sfoster1 merged commit db5c739 into edge Nov 15, 2023
64 of 65 checks passed

sfoster1 deleted the separate-lt-and-ht-pickup-and-dropoff branch November 15, 2023 22:56

		presses: int = Field(
		default=0.0, description="The number of tries required to force pick up a tip."

		@@ -359,6 +359,16 @@ class Config:
		}


		class PipetteRowDefinition(BaseModel):

chore: Separate lt and ht pickup and dropoff #13962

chore: Separate lt and ht pickup and dropoff #13962

Conversation

sfoster1 commented Nov 9, 2023 • edited Loading

Review requests

Testing

codecov bot commented Nov 9, 2023 • edited Loading

Codecov Report

Laura-Danielle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahiuchingau left a comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laura-Danielle left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laura-Danielle Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Laura-Danielle left a comment

Choose a reason for hiding this comment

sfoster1 commented Nov 9, 2023 •

edited

Loading

codecov bot commented Nov 9, 2023 •

edited

Loading

Laura-Danielle left a comment •

edited

Loading

Laura-Danielle Nov 14, 2023 •

edited

Loading