Skip to content

Conversation

@frode-aarstad
Copy link
Contributor

Issue
Resolves #12005

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)

  • 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 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')

@frode-aarstad frode-aarstad marked this pull request as draft January 12, 2026 07:48
@frode-aarstad frode-aarstad self-assigned this Jan 12, 2026
@frode-aarstad frode-aarstad force-pushed the write-everest-controls-using-polars2 branch from e87d8ac to f18c59d Compare January 12, 2026 07:53
@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2026

Codecov Report

❌ Patch coverage is 94.11765% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.64%. Comparing base (87e6aa7) to head (5a82077).
⚠️ Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
src/ert/config/everest_control.py 88.46% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12617      +/-   ##
==========================================
+ Coverage   90.54%   90.64%   +0.10%     
==========================================
  Files         435      429       -6     
  Lines       30014    29823     -191     
==========================================
- Hits        27175    27034     -141     
+ Misses       2839     2789      -50     
Flag Coverage Δ
cli-tests 37.56% <43.13%> (+0.29%) ⬆️
gui-tests 69.37% <43.13%> (+0.65%) ⬆️
performance-and-unit-tests 73.97% <76.47%> (+0.08%) ⬆️
test 38.12% <90.19%> (+0.07%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 12, 2026

Merging this PR will not alter performance

✅ 22 untouched benchmarks


Comparing frode-aarstad:write-everest-controls-using-polars2 (5a82077) with main (f0d9de4)

Open in CodSpeed

@frode-aarstad
Copy link
Contributor Author

frode-aarstad commented Jan 12, 2026

Should not need migration since the only thing Everviz uses from LocalStorage (ert-storage)) is summary data

Copy link

Copilot AI left a comment

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 refactors the EverestControl class to use Polars DataFrames instead of xarray Datasets for storing parameters, aligning it with the storage approach used by other parameter configurations.

Changes:

  • Modified EverestControl.create_storage_datasets() to return Polars DataFrames instead of xarray Datasets
  • Updated parameter loading/writing logic in EverestControl to work with DataFrames
  • Added special handling in LocalEnsemble for everest_parameters type during save/load operations

Reviewed changes

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

Show a summary per file
File Description
src/ert/config/everest_control.py Refactored to use pl.DataFrame instead of xr.Dataset for storage and simplified write_to_runpath logic
src/ert/storage/local_ensemble.py Added special handling for everest_parameters type with DataFrame merging/updating logic
src/ert/config/parameter_config.py Updated return type signature to include config type
src/ert/config/gen_kw_config.py Updated return type and added config type to yield statement
src/ert/config/field.py Updated return type signature to include None for config type
src/ert/config/surface_config.py Updated return type signature to include None for config type

@frode-aarstad frode-aarstad force-pushed the write-everest-controls-using-polars2 branch 2 times, most recently from cb6a4ff to 45c0f07 Compare January 12, 2026 13:43
@frode-aarstad frode-aarstad force-pushed the write-everest-controls-using-polars2 branch from 45c0f07 to 06104bb Compare January 14, 2026 07:12
@frode-aarstad frode-aarstad marked this pull request as ready for review January 14, 2026 07:12
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@frode-aarstad frode-aarstad force-pushed the write-everest-controls-using-polars2 branch from 353b2ae to 17203ca Compare January 14, 2026 07:20
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@frode-aarstad frode-aarstad force-pushed the write-everest-controls-using-polars2 branch from 17203ca to 95c2133 Compare January 14, 2026 09:22
@xjules
Copy link
Contributor

xjules commented Jan 19, 2026

@frode-aarstad you need to make a storage migration. xr.datasets -> pl.Dataframe

df_lazy = self._load_parameters_lazy(SCALAR_FILENAME)
df_lazy = df_lazy.select(["realization", *keys])
names = df_lazy.collect_schema().names()
matches = [key for key in keys if any(key in item for item in names)]
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure about this one. Shall we check if it is present in self.experiment.parameter_keys too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is covered. I added some tests and test_that_load_scalar_keys_raises_key_error_for_unregistered_parameters should cover this, or ?

@jonathan-eq
Copy link
Contributor

@frode-aarstad you need to make a storage migration. xr.datasets -> pl.Dataframe

I think we concluded that this does not require a storage migration

@xjules
Copy link
Contributor

xjules commented Jan 19, 2026

think we concluded that this does not require a storage migration

We are testing it now:

  1. everest run ... with this PR
  2. everest results ... with bleeding

@frode-aarstad
Copy link
Contributor Author

think we concluded that this does not require a storage migration

We are testing it now:

1. `everest run ...` with this PR

2. `everest results ...` with bleeding

Looks good. Works as intended

Copy link
Contributor

@jonathan-eq jonathan-eq left a comment

Choose a reason for hiding this comment

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

Some tiny questions, then we are ready for main!

@frode-aarstad frode-aarstad force-pushed the write-everest-controls-using-polars2 branch from 76e4a27 to 5a82077 Compare January 20, 2026 08:34
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.

Refactor EverestControl to use pl.DataFrame as storage format

4 participants