Skip to content

Remove unused group parameter from load_scalars - #13966

Merged
frode-aarstad merged 2 commits into
equinor:mainfrom
frode-aarstad:remove-load-scalars-group
Jul 11, 2026
Merged

Remove unused group parameter from load_scalars#13966
frode-aarstad merged 2 commits into
equinor:mainfrom
frode-aarstad:remove-load-scalars-group

Conversation

@frode-aarstad

@frode-aarstad frode-aarstad commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Needed for #12784

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 self-assigned this Jul 8, 2026
@frode-aarstad frode-aarstad added the maintenance Not a bug now but could be one day, repaying technical debt label Jul 8, 2026
@frode-aarstad frode-aarstad moved this to In Progress in SCOUT Jul 8, 2026
@codecov-commenter

codecov-commenter commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.38%. Comparing base (ae662b3) to head (3d26c5d).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13966      +/-   ##
==========================================
+ Coverage   91.34%   91.38%   +0.03%     
==========================================
  Files         478      482       +4     
  Lines       34067    34198     +131     
==========================================
+ Hits        31120    31252     +132     
+ Misses       2947     2946       -1     
Flag Coverage Δ
cli-tests 36.01% <ø> (+0.04%) ⬆️
fuzz 43.61% <ø> (+0.08%) ⬆️
gui-tests 59.29% <ø> (+0.08%) ⬆️
performance-and-unit-tests 79.44% <ø> (+0.06%) ⬆️
test 46.10% <ø> (+0.06%) ⬆️

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

Files with missing lines Coverage Δ
src/ert/storage/local_ensemble.py 96.81% <ø> (-0.12%) ⬇️

... and 14 files with indirect coverage changes

@frode-aarstad frode-aarstad moved this from In Progress to Ready for Review in SCOUT Jul 8, 2026
@codspeed-hq

codspeed-hq Bot commented Jul 8, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 36 untouched benchmarks


Comparing frode-aarstad:remove-load-scalars-group (3d26c5d) with main (2a34262)

Open in CodSpeed

@pytest.mark.filterwarnings("ignore:.*Use load_responses.*:DeprecationWarning")
@pytest.mark.filterwarnings("ignore:Config contains a SUMMARY key")
def test_data_fetching_missing_key(snake_oil_case):
def test_that_load_scalars_returns_empty_dataframe_when_no_scalars_exist(

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 changes semantics of the test slightly. Maybe we do have a test for it though; ie. empty dataframe for load_scalars.

@xjules xjules Jul 9, 2026

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.

could you change it to use load_scalar_keys instead? Then the previous test name is ok

@frode-aarstad frode-aarstad Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This a test for load_scalars() . load_scalar_keys() is tested elsewhere

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 updates the storage LocalEnsemble.load_scalars API by removing the unused group parameter and adjusts unit tests/snapshots to reflect the new call pattern. This fits into ERT’s local storage layer by simplifying the scalar-loading interface and keeping tests aligned with the intended API surface.

Changes:

  • Remove the group parameter from LocalEnsemble.load_scalars.
  • Update unit tests to use the new load_scalars() / load_scalars(realizations=...) signature and rename tests to behavior-style names.
  • Refresh snapshot outputs to match the updated test expectations.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/ert/unit_tests/storage/test_save_and_load_responses.py Renames/updates test to validate empty DataFrame behavior using the new load_scalars() signature.
tests/ert/unit_tests/storage/test_save_and_load_parameters.py Updates scalar-loading tests to the new API and refreshes snapshot naming/expectations.
tests/ert/unit_tests/storage/snapshots/test_save_and_load_parameters/test_that_load_scalars_returns_correct_data/0/data.csv New snapshot for full load_scalars() output.
tests/ert/unit_tests/storage/snapshots/test_save_and_load_parameters/test_that_load_scalars_returns_correct_data/0/data_1.csv New snapshot for load_scalars(realizations=[...]) output.
tests/ert/unit_tests/storage/snapshots/test_save_and_load_parameters/test_gen_kw_collector/0/gen_kw_collector_3.csv Removes obsolete snapshot tied to the old test/API usage.
tests/ert/unit_tests/storage/snapshots/test_save_and_load_parameters/test_gen_kw_collector/0/gen_kw_collector_2.csv Removes obsolete snapshot tied to the old test/API usage.
src/ert/storage/local_ensemble.py Removes group filtering from load_scalars and simplifies the implementation accordingly.

Comment thread src/ert/storage/local_ensemble.py

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

Remember to squash the commits.

@github-project-automation github-project-automation Bot moved this from Ready for Review to Reviewed in SCOUT Jul 10, 2026
@frode-aarstad
frode-aarstad merged commit 137d9f2 into equinor:main Jul 11, 2026
35 checks passed
@github-project-automation github-project-automation Bot moved this from Reviewed to Done in SCOUT Jul 11, 2026
@frode-aarstad
frode-aarstad deleted the remove-load-scalars-group branch July 11, 2026 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Not a bug now but could be one day, repaying technical debt

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants