-
Notifications
You must be signed in to change notification settings - Fork 178
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
"namespace": "opentrons", | ||
"byPipette": [ | ||
{ | ||
"pipetteModel": "p10_single", |
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.
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-
- 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. - 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 - 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.
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.
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 [(), ()] |
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.
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?
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.
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?
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.
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
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 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.
5194306
to
3d423c7
Compare
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
Review requests
Risk assessment
None