Skip to content
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(shared-data): update P10 tip totalLiquidVolume #3929

Merged
merged 4 commits into from
Aug 29, 2019

Conversation

ahiuchingau
Copy link
Contributor

overview

The P10 tips can physically hold up to 20 uL rather than 10 uL as described in the opentrons_96_tiprack_10uL definition. When attached to a P20, the 10 uL totalLiquidVolume of the tip is restricting the pipette from aspirating to its full capacity.

changelog

  • increase totalLiquidVolume from 10 to 20

review requests

There's a function that's already taking both the pipette's and tip max volume into account when handling volume logic (see #3674 , #3604). This means that when a P10 tip is used with a P10 pipette, the max volume that the pipette can aspirate is 10 uL; but when used with a P20, the max volume is 20 uL.
The name of the tiprack remains the same for backward compatibility purposes. 🤷‍♀ Any objections?

@ahiuchingau ahiuchingau added ready for review labware shared data Affects the `shared-data` project labels Aug 22, 2019
@ahiuchingau ahiuchingau requested a review from a team August 22, 2019 16:25
@ahiuchingau ahiuchingau force-pushed the sd_update-p10-tiprack-volume branch from 89ffef7 to c06d781 Compare August 22, 2019 18:24
@codecov
Copy link

codecov bot commented Aug 22, 2019

Codecov Report

Merging #3929 into edge will increase coverage by 2.41%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            edge    #3929      +/-   ##
=========================================
+ Coverage   57.3%   59.71%   +2.41%     
=========================================
  Files        822      829       +7     
  Lines      23411    25477    +2066     
=========================================
+ Hits       13415    15214    +1799     
- Misses      9996    10263     +267
Impacted Files Coverage Δ
labware-library/src/filters.js 61.76% <0%> (-6.66%) ⬇️
protocol-designer/src/configureStore.js 2.56% <0%> (-1.79%) ⬇️
opentrons/hardware_control/controller.py 59.44% <0%> (-0.1%) ⬇️
app/src/components/CalibrateLabware/InfoBox.js 0% <0%> (ø) ⬆️
...gner/src/components/LabwareSelectionModal/index.js 0% <0%> (ø) ⬆️
app/src/components/TipProbe/AttachTipPanel.js 0% <0%> (ø) ⬆️
...ocol-designer/src/components/SettingsPage/index.js 0% <0%> (ø) ⬆️
...esigner/src/components/SettingsPage/SettingsApp.js 0% <0%> (ø) ⬆️
app/src/robot/reducer/session.js 100% <0%> (ø) ⬆️
app/src/components/ChangePipette/index.js 0% <0%> (ø) ⬆️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8d0bac...a3a99cb. Read the comment docs.

Copy link

@umbhau umbhau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just capturing an offline conversation:

  • The tiprack currently known as the "P10" (10µl) tiprack will be used for both the existing P10 and new P20 pipettes.
  • This PR updates the labware definition for the tiprack to a 20µl max volume, reflecting the tip's actual max volume, but leaves the name the same (so as not to break old protocols)
  • So, for now, users will have to (counterintuitively) use the 10µl tip with the 20µl pipette.
  • We can improve this in the future by changing the name or creating a second labware definition

Copy link
Contributor

@IanLondon IanLondon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any change to non-metadata stuff in a published labware definition requires version to be incremented

Otherwise existing protocols, both Python and PD protocols that use the def, could unexpectedly change their behavior on the next release of PD/API.

@IanLondon
Copy link
Contributor

IanLondon commented Aug 22, 2019

also this needs to be a new file called 2.json, right now CI is failing due to: FAIL test schemas of all opentrons definitions › file name matches version: /home/travis/build/Opentrons/opentrons/shared-data/labware/definitions/2/opentrons_96_tiprack_10ul/1.json

The original 1.json should not be changed (unless it's a metadata-only change, you always need to make a new {versionHere}.json file and bump the version of a labware def)

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, exposes some deeper issues with labware versioning though.

@Laura-Danielle
Copy link
Contributor

Laura-Danielle commented Aug 26, 2019

Posting here for history sake:

We agreed to add a labware definition named opentrons_96_tiprack_20uL. We will not publish this definition on labware library until p20 gen2 pipettes are ready for purchase on the website. Once this happens we will then mark the p10 definition as deprecated and push users towards the p20 definition.

@sfoster1 sfoster1 self-requested a review August 28, 2019 13:42
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

"brand": "Opentrons",
"brandId": [],
"links": [
"https://shop.opentrons.com/collections/opentrons-tips/products/opentrons-10ul-tips"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we edit the shop so there's a https://shop.opentrons.com/collections/opentrons-tips/products/opentrons-20ul-tips (not 10) and change this link?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add this to the new ticket #3964 when we are ready to publish this definition on the labware library

@ahiuchingau ahiuchingau merged commit f42067e into edge Aug 29, 2019
@ahiuchingau ahiuchingau deleted the sd_update-p10-tiprack-volume branch August 29, 2019 18:05
@mcous mcous mentioned this pull request Sep 9, 2019
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
labware shared data Affects the `shared-data` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants