-
Notifications
You must be signed in to change notification settings - Fork 179
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): inner well geometry unit tests #17082
Open
caila-marashaj
wants to merge
19
commits into
edge
Choose a base branch
from
well-geometry-unit-tests
base: edge
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
+1,923
−1,438
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ryanthecoder
force-pushed
the
well-geometry-unit-tests
branch
from
December 18, 2024 15:18
a3440a9
to
9a0eaf5
Compare
caila-marashaj
force-pushed
the
well-geometry-unit-tests
branch
from
January 6, 2025 15:41
998393f
to
11ee5b7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## edge #17082 +/- ##
===========================================
- Coverage 92.43% 73.82% -18.62%
===========================================
Files 77 43 -34
Lines 1283 3301 +2018
===========================================
+ Hits 1186 2437 +1251
- Misses 97 864 +767
Flags with carried forward coverage won't be shown. Click here to find out more.
|
caila-marashaj
force-pushed
the
well-geometry-unit-tests
branch
from
January 6, 2025 22:46
5f9a620
to
98ea908
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This pr resolves the geometry calculations pertaining to liquid tracking. It updates a lot of the inner well geometry definitions according to the hardware team's measurements, and includes some math and logic fixes.
Frustum Whoopsie
One notable math fix is that the first time
height_to_volume_circular
orvolume_to_height_circular
are called for a given well, we'll now generate a table inshared-data
containing the volume calculated at every height within the well at a 0.005 mm interval. We did this because the inverse function of the polynomial relationship (only for circular frusta) we came up with was shown to have an exponential increase in error with respect to height that peaked at the middle of each well segment.To clarify, finding the volume from the height within a conical frustum works reliably well. This is because the relationship between height and volume takes the following shape:
Volume = a * target_height^3 + b * target_height^2 + c * target_height
, where a, b, and c are found from the heights and radii of the top and bottom faces.This makes finding the volume only a matter of addition and multiplication, but finding the height from some arbitrary volume to require an estimation of the roots of a third-degree polynomial. So, the solution here is just to avoid using that estimation and rely on a set number of the more reliable volume calculations that are done all at once.
For whatever reason, this was only a problem when calculating heights in circular, and not rectangular, frusta, so we left the rectangular math as is.
Changelog