-
Notifications
You must be signed in to change notification settings - Fork 186
fix(shared-data): Sync dimensions and spacing to latest hardware values #19127
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
base: edge
Are you sure you want to change the base?
Conversation
…dsheet, via script. Notes: - Run import_dimensions_from_hw.py. - Manually fix up the output to reduce the diff. e.g. undo changes like 0->0.0. - Opentrons tube rack labware like opentrons_24_tuberack_generic_2ml_screwcap are incomplete in the spreadsheet and couldn't be parsed by the script. They need separate manual attention.
x/y were swapped in innerLabwareGeometry.
These "aluminum block + tube" labware are not mentioned in the hardware spreadsheet. But, they have the same tubes as some "tube rack + tube" labware, which means we can take the diameters from there. * opentrons_24_tuberack_nest_0.5ml_screwcap <- opentrons_24_tuberack_nest_0.5ml_screwcap * opentrons_24_aluminumblock_nest_1.5ml_screwcap <- opentrons_24_tuberack_nest_1.5ml_screwcap * opentrons_24_aluminumblock_nest_1.5ml_snapcap <- opentrons_24_tuberack_nest_1.5ml_snapcap * opentrons_24_aluminumblock_nest_2ml_screwcap <- opentrons_24_tuberack_nest_2ml_screwcap * opentrons_24_aluminumblock_nest_2ml_snapcap <- opentrons_24_tuberack_nest_2ml_snapcap This also makes each definition's tube diameter internally consistent with the diameter implied by its innerLabwareGeometry, yay.
New numbers based on the drawing "SINGLE WELL RESERVOIR SUPPORT DWG" rev A1.0. xDimension 108->108.7: The old value matched the bottom of the well, but a well's xDimension needs to represent the top of the well. 108.7 matches the drawing and the number in innerLabwareGeometry. yDimension 72->70.7: I'm not sure where 72 came from, but 70.7 matches the drawing and the number in innerLabwareGeometry.
We can't easily use a script for this because the spreadsheet doesn't have full information for these definitions--it only has the diameters and depths. Also, some of the tube racks are heterogeneous. Also, we can only copy over the diameter values, not the depth values. Since the hardware spreadsheet doesn't give us a known tube top position, we can't know whether to grow/shrink the tube depths from the bottom or the top.
The hardware spreadsheet doesn't explicitly mention the diameter of these tubes, so this is a little bit dicey. These new numbers make the well diameter internally consistent with the definition's innerLabwareGeometry. A while ago, in PR #17082, this same 27.95 -> 28.18 change was made to opentrons_10_tuberack_nest_4x50ml_6x15ml_conical, which shares the same tubes. I'm guessing this definition was just accidentally omitted.
I've asked @rclarke0 and @HovanNgoOpentrons about these. They'll probably get fixed in a separate PR.
const labwareWithXYDimensionMismatches = [ | ||
// todo(mm, 2025-08-05): Investigate and resolve these mismatches. | ||
'eppendorf_96_wellplate_1000ul', | ||
'nest_8_reservoir_22ml', | ||
] | ||
|
||
if ( | ||
labwareWithXYDimensionMismatches.includes( | ||
labwareDef.parameters.loadName | ||
) | ||
) { | ||
expect(wellDepth).not.toStrictEqual(topFrustumHeight) | ||
expect(xyDimensionsFromWell).not.toStrictEqual(xyDimensionsFromGeometry) | ||
} else { | ||
expect(wellDepth).toStrictEqual(topFrustumHeight) | ||
expect(xyDimensionsFromWell).toStrictEqual(xyDimensionsFromGeometry) | ||
} |
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.
@rclarke0 FYI here's where I ended up with this test. It's currently missing the handling that you wrote for circular sections, but if you end up keeping that, I'll merge it in here somehow.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## edge #19127 +/- ##
==========================================
+ Coverage 24.24% 25.46% +1.22%
==========================================
Files 3370 3371 +1
Lines 296410 294532 -1878
Branches 37690 37678 -12
==========================================
+ Hits 71868 75017 +3149
+ Misses 224518 219486 -5032
- Partials 24 29 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Left comments on files where I saw discrepancies
"yDimension": 71.25, | ||
"totalLiquidVolume": 290000, | ||
"x": 63.88, | ||
"y": 42.74, |
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 google sheet says 42.73
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.
Probably the same thing as #19127 (comment).
"diameter": 16.256, | ||
"totalLiquidVolume": 3400, | ||
"x": 113.96, | ||
"y": 13.79, |
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 sheet says the y offset is 13.78 so I think this should also be 13.78 since its the bottom most well
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.
Probably the same thing as #19127 (comment).
"yDimension": 3.632, | ||
"totalLiquidVolume": 112, | ||
"x": 115.62, | ||
"y": 8.98, |
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 sheet says the Y offset is 8.99
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.
Probably the same thing as #19127 (comment).
"diameter": 11.1, | ||
"totalLiquidVolume": 1600, | ||
"x": 109.72, | ||
"y": 10.12, |
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.
y offset is 10.08 on the sheet
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 is interesting, thanks for calling this out.
According to the spreadsheet:
- y-spacing: 13.08
- y-offset: 10.08
- y-dimension of overall labware: 85.600
If you treat y-offset as the distance from the front of the labware to the center of the "F" row, then this y
value should be 10.08. This is what you're doing.
If you treat y-offset as the offset from the back of the labware to the center of the "A" row, then this y
value should be 85.600 - 10.08 - 13.08*5 = 10.12
. This is what I did. I did this because, as I understand it, the standard way of dimensioning things like this is from the back-left.
I have to imagine that, physically, Corning intends for the well distribution to be exactly symmetrical, evenly dividing the space across 6 rows. Somewhere along the line—maybe Corning's technical drawings, maybe our spreadsheet—the y-spacing or y-offset is getting rounded, and the space is no longer divided exactly evenly.
So our options are:
- Make the y-offset match the spreadsheet in the A-row but not the F-row. (This is what this PR has now.)
- Make the y-offset match the spreadsheet in the F-row but not the A-row.
- Make the y-offset match the spreadsheet in the A-row and the F-row, but then the y-spacing doesn't match the spreadsheet anymore.
My initial reaction is that I'm 50/50 between option 1 and option 2. I don't think we should do option 3.
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.
OK, after looking at the spreadsheet again, I'm leaning towards option 1, because I think that matches how the spreadsheet is thinking about it. In other words, I'm pretty sure the spreadsheet is thinking of y-offset as a measurement from the back of the labware to the center of the "A" row. Notice how in the "ERROR" section, this labware had an error of -0.040, which is corrected here.
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.
Makes sense, I agree with option 1. I think this formula should get added to the PR description so that its clear why the offset numbers in the sheet aren't directly present in the labware definition. I guess this is still a result of the issue that the labware does not assume the top left corner is (0,0).
"diameter": 35.433, | ||
"totalLiquidVolume": 16800, | ||
"x": 103, | ||
"y": 23.19, |
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.
y offset is 23.16
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.
Probably the same thing as #19127 (comment).
shared-data/labware/definitions/2/opentrons_24_tuberack_eppendorf_2ml_safelock_snapcap/3.json
Show resolved
Hide resolved
shared-data/labware/definitions/2/opentrons_6_tuberack_nest_50ml_conical/3.json
Show resolved
Hide resolved
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.
I dont think this needs a 2.json file because it hasn't been released to the public yet
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 just happened today, but #19142, which I think is going into v8.6.0, does release it to the public.
(And from speaking with Seth, this PR will not go into v8.6.0, for risk mitigation reasons. So we gotta leave v1 of this labware alone.)
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.
Reminder to self: This is getting fixed in-place in #19126 so it should probably be removed from this PR.
"yDimension": 71.88, | ||
"totalLiquidVolume": 22000, | ||
"x": 14.38, | ||
"y": 42.71, |
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.
y offset in sheet is 42.76
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.
Probably the same thing as #19127 (comment).
"yDimension": 8.2, | ||
"totalLiquidVolume": 2400, | ||
"x": 14.4, | ||
"y": 11.3, |
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.
y offset on the sheet is 11.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.
Probably the same thing as #19127 (comment).
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.
Thanks for explaining the offsets
Tagging "do not merge": we want to wait until after the v8.6.0 release branch is cut from |
Overview
Closes EXEC-1480.
Changelog
Copies the numbers from the "XY Spacing" tab of the Google Sheet linked in EXEC-1480.
This means updating the coarse-grained labware and well dimensions, but not the fine-grained
innerWellGeometry
.innerWellGeometry
should have already been sorted out by fix(shared-data): SyncinnerLabwareGeometry
to latest hardware values #18266.This was partially script-assisted, but only partially. The details are in the commit messages.
Unfortunately, I couldn't bring the tube racks fully up to date because the spreadsheet is missing some details. It tells us tube depths, but it doesn't tell us anything about where the tube sits vertically. So if we need to fix the depth, we have no way of knowing whether to grow/shrink it from the top or the bottom.
Adjust our labware definition tests to only check each labware's latest version for sensible geometry.
Test Plan and Hands on Testing
Relying on code review and automated labware definition sanity checks to catch any flagrant errors.
Review requests
For reviewability, this is broken up so you can go commit-by-commit to see what actually changed. The consolidated diff is not helpful because it just looks like a million new files.
Basically just double-check my new numbers against the numbers in the Google Sheet and make sure I didn't make any silly mistakes like copying from the wrong place.
Risk assessment
Low. Most of the changes are pretty small, numerically.