-
Notifications
You must be signed in to change notification settings - Fork 184
refactor(api): height from volume binary search #18081
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 #18081 +/- ##
===========================================
- Coverage 57.52% 25.67% -31.85%
===========================================
Files 3043 2938 -105
Lines 255340 227623 -27717
Branches 30544 27497 -3047
===========================================
- Hits 146879 58446 -88433
- Misses 108275 169160 +60885
+ Partials 186 17 -169
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
93d653b
to
13bb526
Compare
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.
there's definitely a limit to the number of checks that we'll require, but it's still generally iffy to do a bunch of recursion in python (sadly!) because it can have a pretty heavy stack frame, so let's make this not recursive.
Let's also get this tested with hypothesis if we can, or ideally with some pretty rigorous numerics testing beyond just the boundary tests.
other than that, looks great!
084d34a
to
3e79491
Compare
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.
This looks great and I'm always hype for hypothesis (#hypeforhypothesis)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
(cherry picked from commit 5af0934)
Overview
For circular frusta, you can use calculus to determine a volume from a height, and not a height from a volume (the equation isolating height is a higher degree polynomial and gets pretty gross to try and solve analytically). The way we were getting around this before was by creating a lookup table during a protocol run, when a labware was loaded with a
CircularFrustum
, at a fixed interval of every 0.5 mm to maintain accuracy. This became a problem for very tall circular frusta (like test tubes), and protocols that used them with this code would silently pause for 2 minutes at the start to fill out these very large tables.This implements a binary search instead to find a height from a volume within a circular frustum, upon request.