-
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(api) Support changing current based on tip configuration #13660
feat(api) Support changing current based on tip configuration #13660
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## feat-partial-tip-configuration #13660 +/- ##
==================================================================
Coverage 71.29% 71.30%
==================================================================
Files 2423 1587 -836
Lines 68201 52852 -15349
Branches 7972 3480 -4492
==================================================================
- Hits 48625 37685 -10940
+ Misses 17704 14617 -3087
+ Partials 1872 550 -1322
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
The code looks good to me! Will have to test on a machine though
cls, | ||
nozzle_map: Dict[str, List[float]], | ||
default_pickup_current: float, | ||
pick_up_current_map: Optional[Dict[int, float]] = None, |
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.
in what situations would we not have a pick_up_current_map
? does it have to be = None
?
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.
single channels do not have a pick up current map.
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.
Review now with appropriate resolution selected
7603ab4
to
6eab794
Compare
"5": 0.6, | ||
"6": 0.7, | ||
"7": 0.8, | ||
"8": 0.9 |
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.
are these real values? if not let's get those
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.
Also is the current value defined in the pickUpTipConfigurations
being used anywhere anymore?
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 these are not real numbers. pickUpTipConfigurations
current will not be used anymore.
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.
From HW:
Let's change the values for the perTipPickupCurrent
configurations such that:
- For 1-channel pickups, the current is the same as the pickup current for the single-channel version of the pipette
- For 2-7 channel pickups inclusive, the current is
Ntips / Nchannels
times the as-of-now 8channel pickup current - For 8 channel pickups, the current is the same is the as-of-now 8channel pickup current
These will be good for the OT-2 pipettes (the different nozzle geometry means the overlap positioning won't change much), and they'll be good starting points for more in-depth testing of the Flex configurations.
From me:
We also should probably get rid of pickUpTipConfigurations.current
.
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 looks good, will try and get this tested!
shared-data/pipette/definitions/2/general/eight_channel/p1000/1_0.json
Outdated
Show resolved
Hide resolved
return self._current_scalar | ||
def get_tip_configuration_current(self) -> float: | ||
return self._pick_up_current_map[ | ||
len(self._current_nozzle_configuration.map_store) |
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.
nit: i would love it if the nozzle configuration objects had like tip_count
property that did this or something but it's ok if it works
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.
sure can do that
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.
looks good on hardware!
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.
Ah, looks like @caila-marashaj did some in person testing so we're good - let's just fix that malformed mu
0302955
into
feat-partial-tip-configuration
Overview
With this PR, we should now be able to perform partial tip configuration movements in the hardware controller.
Test Plan
We should also check that these movements persist for liquid handling functions as well.
Changelog
Review requests
HW folks, please comment and tell me what the partial currents should actually be. I made up numbers here.
SW folks, let me know if anything seems weird/gross/whatever.
Risk assessment
Medium. We should ensure that normal configuration tip pick-ups have not deviated from normal.