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

Merged
merged 9 commits into from
Nov 18, 2024

Conversation

sanni-t
Copy link
Member

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

Addresses AUTH-833

Overview

Adds liquid class definition values for water for use with all Flex pipette and combos

Test Plan and Hands on Testing

  • TODO in a follow up PR: add a test to verify schema compatibility of the definition
  • For now the definition checks we have will suffice

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
@sanni-t sanni-t marked this pull request as ready for review November 15, 2024 21:01
@sanni-t sanni-t requested review from a team as code owners November 15, 2024 21:01
Copy link
Contributor

@jbleon95 jbleon95 left a comment

Choose a reason for hiding this comment

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

Code changes LGTM, and if the definition is passing our unit tests than that should be good to go too.

self._sorted_volumes, self._sorted_values = (
zip(*sorted(self._properties_by_volume.items()))
if len(self._properties_by_volume) > 0
else [(), ()]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the empty list of properties something we want to support? Wouldn't that break the getter function if there's nothing at all to interpolate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ughh! You're right, this will break the getter function. But we do want to allow empty list when there's only a default value specified. The default value should apply to everything. @jbleon95 any thoughts about this and why the getter function doesn't support default-only values right now?

Copy link
Contributor

Choose a reason for hiding this comment

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

No good reason other than me not thinking about that scenario, and we'll clear up exact behavior later today, but right now I'm thinking

  • No per volume properties and a default: return default for all volumes
  • No per volume porperties and no default: either raise an error, or disallow removing a per-volume value if it's the only one left

Copy link
Member Author

Choose a reason for hiding this comment

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

OK so the decision is that we'll remove 'default' keys altogether. The schema will need to be updated and it will require at least one volume point in all the 'by volume' properties.
Going to leave this as is and merge this PR and have a follow-up PR for default keys removal.

@ryanthecoder ryanthecoder force-pushed the shared-data-add_liquid_class_definitions branch from 5194306 to 3d423c7 Compare November 18, 2024 16:10
@sanni-t sanni-t merged commit 9fceef6 into edge Nov 18, 2024
54 checks passed
@sanni-t sanni-t deleted the shared-data-add_liquid_class_definitions branch November 18, 2024 22:30
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.

5 participants