Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set ensemble_size dynamically for each batch #9012

Conversation

StephanDeHoop
Copy link
Contributor

@StephanDeHoop StephanDeHoop commented Oct 21, 2024

Issue
Resolves #8848

Approach
Remove the default number of realizations and set it dynamically for each batch when a new ensemble is created. Also, for completeness still setting number of realizations equal to length of the realization list in the config file (in Ert AnalysisConfig and ModelConfig we still use the config dictionary and ConfigKeys.NUM_REALIZATIONS to set the num_realizations field). If we know how many realizations there are, why not set is correctly was my logic. Also cleaned up the hard coded strings by importing the Ert ConfigKeys.

(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 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')

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: Create Backport PR to latest release

@codecov-commenter
Copy link

codecov-commenter commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.96%. Comparing base (1315c55) to head (986cce5).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9012      +/-   ##
==========================================
+ Coverage   90.86%   90.96%   +0.10%     
==========================================
  Files         349      352       +3     
  Lines       21621    21695      +74     
==========================================
+ Hits        19645    19734      +89     
+ Misses       1976     1961      -15     
Flag Coverage Δ
cli-tests 39.08% <ø> (-0.05%) ⬇️
gui-tests 72.71% <ø> (-0.08%) ⬇️
performance-tests 49.58% <ø> (-0.10%) ⬇️
unit-tests 79.69% <ø> (+0.03%) ⬆️

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.

@StephanDeHoop StephanDeHoop force-pushed the remove_default_ensemble_size_in_everest branch 2 times, most recently from 827d39b to a52f1c2 Compare October 21, 2024 14:49
@StephanDeHoop StephanDeHoop marked this pull request as ready for review October 21, 2024 15:25
@StephanDeHoop StephanDeHoop marked this pull request as draft October 21, 2024 15:26
@StephanDeHoop StephanDeHoop marked this pull request as ready for review October 22, 2024 06:38
Copy link
Contributor

@verveerpj verveerpj left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@yngve-sk yngve-sk left a comment

Choose a reason for hiding this comment

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

I would rebase the commits so you have the introducing of ConfigKeysErt completely separate from the changing of the default value, the commit with the test fixup now relates to both of these things, it would be nice to have it in sequence and not intermingled. Maybe a bit of a nitpick but I'm not sure about this practice of using these "string enums" in general to access the dicts.

@verveerpj
Copy link
Contributor

I would rebase the commits so you have the introducing of ConfigKeysErt completely separate from the changing of the default value, the commit with the test fixup now relates to both of these things, it would be nice to have it in sequence and not intermingled. Maybe a bit of a nitpick but I'm not sure about this practice of using these "string enums" in general to access the dicts.

That is a very good point. Should we really use these enums? Maybe we can discuss this in the standup.

@StephanDeHoop StephanDeHoop force-pushed the remove_default_ensemble_size_in_everest branch from b1e2d6e to 986cce5 Compare October 23, 2024 08:02
@StephanDeHoop StephanDeHoop merged commit ca44e00 into equinor:main Oct 23, 2024
56 checks passed
@StephanDeHoop StephanDeHoop deleted the remove_default_ensemble_size_in_everest branch October 23, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the default of 10000 realizations
4 participants