Skip to content
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

volume at arbitrary height for most above spherical section heights #16311

Merged
merged 13 commits into from
Sep 25, 2024

Conversation

caila-marashaj
Copy link
Contributor

@caila-marashaj caila-marashaj commented Sep 19, 2024

Overview

Here are the protocol engine functions to find the volume or height at any arbitrary height/volume within a well.

Changelog

  • add functions to find non-border heights and volumes in geometry.py
  • logic to look through every section of a well for our target height/volume
  • SphericalSegment.radius_of_curvature should have been radiusOfCurvature
  • added a height to volume function using the volume-of-a-frustum formula

TODO

I'm going to add tests in a follow-up pr if that's alright. I want to get these functions merged first though so I can unblock Peter's pr

@caila-marashaj caila-marashaj force-pushed the volume-at-arbitrary-height branch from 93560c0 to 06cdf39 Compare September 24, 2024 21:19
@caila-marashaj caila-marashaj marked this pull request as ready for review September 25, 2024 16:27
@caila-marashaj caila-marashaj requested a review from a team as a code owner September 25, 2024 16:27
Copy link
Contributor

@ryanthecoder ryanthecoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

Comment on lines 69 to 74
def height_from_frustum_formula(area_1: float, area_2: float, volume: float) -> float:
"""Get the volume within a section with differently shaped boundary cross-sections."""
area_term = area_1 + area_2 + sqrt(area_1 * area_2)
return 3 * volume / area_term


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This doesn't look like it's currently used for anything. It sounds like that's intentional, just making sure.

  2. Is this docstring right? The function and variable names make it sound like it's supposed to return a height, not a volume.

  3. Assuming that this is supposed to return a height, given a volume and top and bottom areas:

    I don't think this is mathematically correct for the kinds of frusta that we have. Experimentally, it looks correct for pyramidal frusta but not for non-pyramidal frusta. I can prepare a CAD demo in a sec.

    This might be the same thing that we (@caila-marashaj+@ryanthecoder+me) talked about a few weeks ago in the office. There are "frustum formulas", but if you look at their precise definitions and conditions of use, they only describe pyramidal frusta, and well shapes are frequently not pyramidal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recap from call:

  • Yeah, we think the math is inapplicable, and we'll delete this for now.
  • The plan was for this to help with EXEC-712. We'll make sure the pyramidal assumption hasn't leaked into the rest of the math there.
  • I haven't reviewed the rest of this, but LGTM given @ryanthecoder's approval.

@SyntaxColoring SyntaxColoring dismissed their stale review September 25, 2024 20:40

See thread above

@caila-marashaj caila-marashaj force-pushed the volume-at-arbitrary-height branch from ec37de5 to 7f13592 Compare September 25, 2024 21:07
@caila-marashaj caila-marashaj merged commit 04439fe into edge Sep 25, 2024
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants