Skip to content

Fix bugs in well_state *RateSI() unit conversion#1212

Merged
eivindjahren merged 1 commit into
mainfrom
fix_si_units_test
Jun 26, 2026
Merged

Fix bugs in well_state *RateSI() unit conversion#1212
eivindjahren merged 1 commit into
mainfrom
fix_si_units_test

Conversation

@eivindjahren

@eivindjahren eivindjahren commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

The rates would be off due to a macro expansion bug. Also, in ebd5ddd, the unit_system is now initialized in RSTHead, so prior to that commit, SI units were always equal to the raw units.

Resolves #1213

@eivindjahren eivindjahren force-pushed the fix_si_units_test branch 2 times, most recently from af6352b to 7bcccc8 Compare June 26, 2026 07:26
@eivindjahren eivindjahren self-assigned this Jun 26, 2026
@eivindjahren eivindjahren moved this to Fast Track in SCOUT Jun 26, 2026
Comment thread tests/test_well.py Outdated
LAB_UNITS = 3

# Unit conversion constants
INCH = 0.0254

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For readability, append # meters

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Comment thread tests/test_well.py Outdated
LITER = 0.001
MILLI_LITER = LITER * 0.001
GAS_FIELD = (FEET**3) * 1_000_000
HOUR = 3600

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

append # seconds

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Comment thread tests/test_well.py Outdated
FEET = 12 * INCH
GALLON = 231 * INCH**3
BARREL = GALLON * 42
LITER = 0.001

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Append # m^3

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Comment thread tests/test_well.py Outdated
BARREL = GALLON * 42
LITER = 0.001
MILLI_LITER = LITER * 0.001
GAS_FIELD = (FEET**3) * 1_000_000

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggest to rename this variable to MMCF

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Comment thread tests/test_well.py Outdated
# The reservoir volume rate is unit-independent and equals its SI value.

# The reservoir volume rate is implemented as being unit-independent,
# I have not confirmed that this is correct

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is understood now?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yup, fixed

@eivindjahren eivindjahren changed the title Fix a bug in well_state *RateSI() unit conversion Fix bugs in well_state *RateSI() unit conversion Jun 26, 2026
Comment thread lib/include/resdata/rd_units.hpp Outdated
#ifdef __cplusplus
}

inline double liquid_conversion_rate(ert_rd_unit_enum unit_system) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rename to liquid_conversion_factor?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Comment thread lib/include/resdata/rd_units.hpp Outdated
double conversion_factor = 1;

if (unit_system == RD_METRIC_UNITS)
conversion_factor = 1.0 / RD_UNITS_TIME_DAY;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(I guess you could return directly in each if-statement)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Comment thread lib/include/resdata/rd_units.hpp Outdated
return conversion_factor;
}

inline double gas_conversion_rate(ert_rd_unit_enum unit_system) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rate -> factor

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

@berland berland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

merge at will (friday-deployday!)

@github-project-automation github-project-automation Bot moved this from Fast Track to Reviewed in SCOUT Jun 26, 2026
@eivindjahren eivindjahren requested a review from Copilot June 26, 2026 14:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR corrects SI unit conversion for well and connection rate accessors (*RateSI()) by fixing conversion-factor handling (including a macro expansion precedence issue) and by ensuring the declared restart-file unit system is used when computing SI rates. It also replaces an Equinor-data-based test with self-contained restart-file writer tests to validate rate conversions across unit systems.

Changes:

  • Fix SI conversions for well-state and well-connection rates (including reservoir volume rates) based on the restart file’s unit system.
  • Add unit-system-aware restart writer utilities/tests (XWEL + XCON) to validate SI scaling for METRIC/FIELD/LAB.
  • Refactor/centralize C++ conversion factors and correct XWEL index constants used for oil/water/gas rates.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/well_tests/test_rd_well3.py Removes legacy Equinor dataset-based rate assertions.
tests/test_well.py Adds unit-system-aware restart writing (INTEHEAD unit system + XCON) and new parametrized SI conversion tests.
python/resdata/well/well_state.py Documents raw-vs-SI semantics for rate getters.
lib/resdata/well_state.cpp Uses shared conversion helpers for SI rates and applies conversion to volume_rate_si; fixes XWEL oil index usage via constant update.
lib/resdata/well_conn.cpp Stores unit system on connections and applies SI conversions to connection rates.
lib/resdata/tests/well_segment_conn.cpp Updates test allocations to pass unit system.
lib/resdata/tests/well_conn.cpp Updates test allocations to pass unit system.
lib/include/resdata/well/well_const.hpp Corrects XWEL_RES_ORAT_ITEM index.
lib/include/resdata/well/well_conn.hpp Extends allocation API to accept unit_system.
lib/include/resdata/rd_units.hpp Fixes macro precedence with parentheses and adds shared conversion-factor helpers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/resdata/well_conn.cpp
Comment thread lib/resdata/well_conn.cpp
Comment thread lib/include/resdata/rd_units.hpp
Comment thread lib/include/resdata/well/well_conn.hpp
There were a number of bugs with the *RateSI() function

1. The conversion rates would be off due to a macro expansion bug.

2 in ebd5ddd, the unit_system
is now initialized in RSTHead, so prior to that commit, SI
units were always equal to the raw units.

3) by outputting the WVPR with OPM flow, which should correspond to
volumeRate, we have confirmed that volumeRate should have
the same conversion factor as oil. Before the conversion rate
was always 1.0.

4) by looking at a well with one connection we got values in XCON
that matched XWEL with OPM flow. Which confirms that these values
have the same units and the corresponding conversion should be done
for rateSI() in WellConnection. Before the raw value was used.
@eivindjahren eivindjahren merged commit ebfff13 into main Jun 26, 2026
12 checks passed
@eivindjahren eivindjahren deleted the fix_si_units_test branch June 26, 2026 15:39
@github-project-automation github-project-automation Bot moved this from Reviewed to Done in SCOUT Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Investigate conversion rate behavior

3 participants