Skip to content

Assure breakthrough time is always cast to ms - #14207

Merged
achaikou merged 1 commit into
equinor:mainfrom
achaikou:none_breakthrough
Aug 19, 2026
Merged

Assure breakthrough time is always cast to ms#14207
achaikou merged 1 commit into
equinor:mainfrom
achaikou:none_breakthrough

Conversation

@achaikou

Copy link
Copy Markdown
Contributor

Issue
Resolves #13962

Approach
In theory we could fix this also in migration, but I think this issue is not so bad to warrant it.

Could backport to more previous releases, IF I figure out which release I need to backport it to...

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'just rapid-tests')

When applicable

  • When screenshots are changed: Review screenshot-PR in ert-testdata,
    merge screenshot-PR in ert-testdata before merging this PR.
  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Add backport label to latest release (format: 'backport release-branch-name')

@achaikou achaikou added release-notes:bug-fix Automatically categorise as bug fix in release notes backport version-24.0 labels Aug 18, 2026
@achaikou
achaikou requested a lite review from Copilot August 18, 2026 16:37

Copilot AI 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.

Pull request overview

This PR fixes a storage-read failure for breakthrough responses by ensuring the time column is always stored with a consistent Polars datetime unit (ms), even when breakthrough time is None for a realization. This aligns breakthrough response frames across realizations so they can be concatenated/loaded without schema conflicts.

Changes:

  • Ensure breakthrough time series is always cast to Datetime('ms'), including the “all None” case.
  • Introduce an explicit response_schema() for breakthrough responses and assert it before returning derived frames.
  • Add a regression test that saves/loads breakthrough responses across realizations with and without a breakthrough time.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/ert/config/breakthrough_config.py Forces time to ms and asserts a stable schema for derived breakthrough responses.
tests/ert/unit_tests/config/test_breakthrough_config.py Regression test covering stacking/loading breakthrough frames when a realization has None breakthrough time.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@codecov-commenter

codecov-commenter commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.94%. Comparing base (47757c5) to head (e1528d0).
⚠️ Report is 15 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14207   +/-   ##
=======================================
  Coverage   91.93%   91.94%           
=======================================
  Files         482      484    +2     
  Lines       33516    33612   +96     
=======================================
+ Hits        30813    30903   +90     
- Misses       2703     2709    +6     
Flag Coverage Δ
cli-tests 36.45% <40.00%> (-0.03%) ⬇️
fuzz 44.41% <40.00%> (-0.05%) ⬇️
gui-tests 58.74% <40.00%> (-0.11%) ⬇️
performance-and-unit-tests 80.94% <100.00%> (+0.05%) ⬆️
test 46.01% <40.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/ert/config/breakthrough_config.py 97.87% <100.00%> (+0.19%) ⬆️

... and 10 files with indirect coverage changes

@achaikou
achaikou marked this pull request as ready for review August 18, 2026 17:21
@codspeed-hq

codspeed-hq Bot commented Aug 18, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 36 untouched benchmarks


Comparing achaikou:none_breakthrough (e1528d0) with main (a04d7f4)

Open in CodSpeed

@ajaust ajaust self-assigned this Aug 19, 2026

@ajaust ajaust 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.

Regarding the backporting. I assume backporting to the latest release should be fine (like you are doing by tagging the PR). If we realise that we need to backport the fix even further, we can add more labels later (afaik even after merging) and this will create backport PRs.

Comment thread src/ert/config/breakthrough_config.py

@ajaust ajaust 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.

I just forgot to add the comment about the test assertion to the initial review. 👴

Comment thread tests/ert/unit_tests/config/test_breakthrough_config.py Outdated

@ajaust ajaust 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.

Looks good. 🎉

@achaikou
achaikou merged commit 293883c into equinor:main Aug 19, 2026
35 checks passed
@scout-team-app

Copy link
Copy Markdown

Successfully created backport PR for version-24.0:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport version-24.0 release-notes:bug-fix Automatically categorise as bug fix in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Breakthrough realization with None breakthrough time saves dataframe with unfitting schema

4 participants