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

feat(shared-data): add liquid class definition for water #16671

Draft
wants to merge 2 commits into
base: edge
Choose a base branch
from

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Nov 2, 2024

WIP

Addresses AUTH-833

Overview

Adds definition for water.

I have added placeholder values for all OT2 and Flex pipettes with their respective compatible, regular Opentrons tipracks (non-filter). @andySigler will be adding the actual values.

Test Plan and Hands on Testing

  • TODO: add a test to verify schema compatibility of the definition

Review requests

  • see questions in comments

Risk assessment

None

Copy link

codecov bot commented Nov 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.36%. Comparing base (094be6e) to head (de8d777).
Report is 21 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             edge   #16671       +/-   ##
===========================================
- Coverage   92.43%   79.36%   -13.08%     
===========================================
  Files          77      120       +43     
  Lines        1283     4493     +3210     
===========================================
+ Hits         1186     3566     +2380     
- Misses         97      927      +830     
Flag Coverage Δ
g-code-testing 92.43% <ø> (ø)
shared-data 74.14% <100.00%> (?)

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

Files with missing lines Coverage Δ
...n/opentrons_shared_data/liquid_classes/__init__.py 80.00% <100.00%> (ø)

... and 42 files with indirect coverage changes

"namespace": "opentrons",
"byPipette": [
{
"pipetteModel": "p10_single",
Copy link
Member Author

Choose a reason for hiding this comment

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

pipetteModel is not appropriate for the value we are using here- which is the API load name of a pipette.
We should do one of these things-

  1. Call this field pipetteLoadName so that it is more appropriate. Keeping the pipette identity be pipette load name makes it straightforward for if & when we allow custom liquid class definition files. Users know the pipettes by their API load names and hence will find it intuitive to use the load name. But, this will make it a bit convoluted for PD because PD uses engine-relevant pipette naming.
  2. Keep pipetteModel and use true model names, i.e., p10, p20, p300, etc. This will only work (and in fact would be preferred) if the liquid class values are identical for all generations of the given pipette model
  3. Change to pipetteName which will be the name used in all layers except the PAPI layer. This name is the same as API load name for the OT2 pipettes. It diverges from the API load names of Flex pipettes.
    Pros: More straightforward when using PD, and consistent with the name that most of our backend uses
    Cons: Will be confusing if/when we allow users to create their own custom definitions files. But maybe that won't be a problem if the primary way a user will create custom liquid class definitions is by using some Opentrons software (custom liquid definitions creator?). Also this won't pose much of a problem when defining custom liquid classes in the API because we can always do the PAPI-load-name <-> PE-load-name conversion.

@sanni-t sanni-t changed the title feat(shared-data): add water liquid class definition feat(shared-data): add liquid class definition for water Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant