Skip to content

fix(api): use same well offset between liquid measuring functions#18323

Merged
caila-marashaj merged 4 commits intoedgefrom
use-same-offset-for-measuring-liquid
May 22, 2025
Merged

fix(api): use same well offset between liquid measuring functions#18323
caila-marashaj merged 4 commits intoedgefrom
use-same-offset-for-measuring-liquid

Conversation

@caila-marashaj
Copy link
Copy Markdown
Contributor

Overview

When using either liquid_probe_with_recovery, liquid_probe_without_recovery, or detect_liquid_presence in the engine layer, use the same well offset to begin the liquid probe from, which is 2mm above the top of the well.

@caila-marashaj caila-marashaj requested a review from a team as a code owner May 12, 2025 19:07
@caila-marashaj caila-marashaj added the gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails label May 12, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.84%. Comparing base (dbf0a39) to head (1dba6c1).
Report is 1 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             edge   #18323   +/-   ##
=======================================
  Coverage   24.84%   24.84%           
=======================================
  Files        3225     3225           
  Lines      273249   273249           
  Branches    25995    25995           
=======================================
  Hits        67887    67887           
  Misses     205342   205342           
  Partials       20       20           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Copy Markdown
Contributor

A PR has been opened to address analyses snapshot changes. Please review the changes here: #18324

…quid snapshots (#18324)

This PR was requested on the PR
#18323

Co-authored-by: caila-marashaj <98041399+caila-marashaj@users.noreply.github.com>
@caila-marashaj caila-marashaj requested a review from a team as a code owner May 12, 2025 20:27
Copy link
Copy Markdown
Contributor

@CaseyBatten CaseyBatten left a comment

Choose a reason for hiding this comment

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

Looks good. I'd suggest calling the defined LIQUID_PROBE_START_OFFSET_FROM_WELL_TOP in your unit test instead of just adding the 2mm offset incase it changes in the future, or maybe a test rewrite that verifies that target position was modified by the defined probe offset, no biggie either way.

@ddcc4
Copy link
Copy Markdown
Contributor

ddcc4 commented May 13, 2025

I'd suggest calling the defined LIQUID_PROBE_START_OFFSET_FROM_WELL_TOP in your unit test instead of just adding the 2mm offset incase it changes in the future

Side note to @sfoster1: this is another use case for the hypothetical "share constants between Python and JS" mechanism, because if this DOES change in the future, we'd have to update the corresponding constant in PD step generation too.

@caila-marashaj caila-marashaj merged commit b992d0a into edge May 22, 2025
26 checks passed
ddcc4 pushed a commit that referenced this pull request May 22, 2025
ddcc4 pushed a commit that referenced this pull request May 23, 2025
ddcc4 pushed a commit that referenced this pull request May 24, 2025
ddcc4 pushed a commit that referenced this pull request May 24, 2025
ddcc4 pushed a commit that referenced this pull request May 29, 2025
ddcc4 pushed a commit that referenced this pull request May 29, 2025
ddcc4 pushed a commit that referenced this pull request May 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gen-analyses-snapshot-pr Generate a healing PR if the analyses snapshot test fails

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants