-
Notifications
You must be signed in to change notification settings - Fork 121
Rename ExtParamConfig to EverestControl #12554
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
Rename ExtParamConfig to EverestControl #12554
Conversation
There was a problem hiding this 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 renames the ExtParamConfig class to EverestControl to better reflect that it is exclusively used by Everest and make its purpose more explicit. This is a straightforward refactoring to improve code clarity.
- Class renamed from
ExtParamConfigtoEverestControlin the main definition - All import statements updated across the codebase
- Type annotations and hints updated in all affected files
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/ert/config/everest_control.py | Renamed class from ExtParamConfig to EverestControl and updated docstring |
| src/ert/config/init.py | Updated imports and exports to use EverestControl instead of ExtParamConfig |
| src/ert/config/ensemble_config.py | Updated import and type hint in EnsembleConfig |
| src/ert/storage/local_experiment.py | Updated import and type adapter annotation |
| src/ert/run_models/initial_ensemble_run_model.py | Updated import and type annotation |
| src/ert/run_models/everest_run_model.py | Updated imports, type hints in ext_param_configs property and _create_optimizer method |
| src/ert/run_models/_create_run_path.py | Updated import and isinstance check |
| src/everest/config/control_config.py | Updated import and return type of to_ert_parameter_config() method |
| src/everest/config/utils.py | Updated import and function parameter type hint |
| src/everest/optimizer/everest2ropt.py | Updated import and function parameter type hints |
| src/everest/optimizer/opt_model_transforms.py | Updated import and function parameter type hint |
| tests/ert/unit_tests/storage/test_local_storage.py | Updated import and test instantiation |
7cd95ad to
ad67efd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12554 +/- ##
=======================================
Coverage 90.62% 90.63%
=======================================
Files 432 432
Lines 29738 29730 -8
=======================================
- Hits 26951 26945 -6
+ Misses 2787 2785 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ad67efd to
d295f7e
Compare
CodSpeed Performance ReportMerging #12554 will not alter performanceComparing Summary
|
|
What about storage migration with the renaming? Not sure though if everest actually needs it. |
I think that is taken care of by the to12 migrations. |
| # There will and must always be one EverestControl config for an | ||
| # Everest optimization. | ||
| return cast(list[ExtParamConfig], ext_params) | ||
| return cast(list[EverestControl], controls) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this cast required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
|
|
||
|
|
||
| def test_that_controls_ordering_is_the_same_for_ropt_and_extparam(): | ||
| def test_that_controls_ordering_is_the_same_for_ropt_and_everest_control(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, not sure if this makes the test name more readable 😄
xjules
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @frode-aarstad !
Don't forget to squash the commits.
🚀
Issue
Resolves #12158
Approach
Short description of the approach
(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'just rapid-tests')When applicable