-
Notifications
You must be signed in to change notification settings - Fork 183
feat(step-generation): py command generation for mix() #17833
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #17833 +/- ##
===========================================
+ Coverage 23.41% 62.52% +39.11%
===========================================
Files 2891 2921 +30
Lines 222561 226685 +4124
Branches 19013 19793 +780
===========================================
+ Hits 52115 141739 +89624
+ Misses 170435 84761 -85674
- Partials 11 185 +174
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
repetitions=2, | ||
volume=5, | ||
location=mockPythonName["A1"].bottom(z=3.2), | ||
rate=(2.1 / mockPythonName.flow_rate.aspirate) + (2.2 / mockPythonName.flow_rate.dispense), |
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.
need to figure out how to show both flow rates or if we have to remove one?
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.
okay discussed with jeremy in standup and actually we can just define it above the mix()
like this
mockPythonName.flow_rate.aspirate = 35
mockPythonName.flow_rate.dispense = 57
mockPythonName.mix(
repetitions=2,
volume=5,
location=mockPythonName["C1"].bottom(z=3.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.
Huh, that's a very creative approach!
I think it would work. One potential negative is that the pipette.flow_rate
s are going to jump around a lot during the course of a protocol, when I think the original intent was that they should be used to set a general default and not change so often. But I think we can live with this, since this is the only way to configure a different aspirate and dispense flow rate for the mix()
.
mockPythonName.dispense(...) | ||
mockPythonName.flow_rate.aspirate = 3.78 | ||
mockPythonName.flow_rate.dispense = 3.78 | ||
mockPythonName.mix(...) |
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.
Oh wow, we already had a test for a Mix step. That's nice :)
// If delay is specified, emit individual py commands | ||
// otherwise, emit mix() | ||
const hasUnsupportedMixApiArg = | ||
aspirateDelaySeconds != null || dispenseDelaySeconds != null |
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.
Does this need to be a null
check? Like, can we still call the PAPI mix()
function if aspirateDelaySeconds
is set to 0
?
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.
oh, i have not considered someone setting the delay seconds to 0. I think you're right. The seconds is null
if the user never touched the field so i guess we need to check for both.
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, this looks good.
My only outstanding question is whether aspirateDelaySeconds != null || dispenseDelaySeconds != null
is the right thing to check for.
closes AUTH-1097
Overview
this PR introduces the py command generation for
mix()
! It is emitted only if there are acceptable args. Meaning no aspirate delay, no dispense delay.TODO: figure out the flow rates! user can specify both aspirate and dispense flow rates but mix can only use one. how do we know which one to use?
Test Plan and Hands on Testing
Review the code and smoke test
here is an example i made that passes analysis for the different variations of commands
Changelog
mix()
in the mixUtilRisk assessment
low, behind a ff