Conversation
yngve-sk
force-pushed
the
26.02.unroll-everestcontrols
branch
from
55643db to
8245761
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12857 +/- ##
==========================================
- Coverage 90.64% 90.63% -0.01%
==========================================
Files 440 441 +1
Lines 30460 30488 +28
==========================================
+ Hits 27610 27634 +24
- Misses 2850 2854 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…ummary of the changes: Summary I've successfully completed the refactoring requested in issue equinor#12853: Unroll input_keys in EverestControl. Here's what was changed: Key Changes: 1. EverestControl Structure (src/ert/config/everest_control.py): - Changed from representing multiple variables (list fields) to a single scalar variable - Replaced list fields with scalar equivalents: - input_keys → input_key (single string) - types → control_type_ (single value) - initial_guesses → initial_guess (single float) - control_types → variable_type (single value) - enabled → is_enabled (single bool) - min → min_value (single float) - max → max_value (single float) - perturbation_types → perturbation_type (single value) - perturbation_magnitudes → perturbation_magnitude (single float) - scaled_ranges → scaled_range (single tuple) - samplers → sampler (single SamplerConfig or None) - Added group field to reference the control group name - Updated __len__() to always return 1 - Updated create_storage_datasets() to work with a single value - Removed write_to_runpath() logic (now handled at higher level) 2. ControlConfig.to_ert_parameter_config() (src/everest/config/control_config.py): - Changed return type from EverestControl to list[EverestControl] - Creates one EverestControl object per variable (unrolled structure) - Each control has its own unique name (e.g., "point.x", "point.y", "point.z") - All share the same group name (e.g., "point") - All share the same output_file 3. Updated Optimizer Code: - everest2ropt.py: Updated to work with scalar fields instead of nested loops - opt_model_transforms.py: Updated to use scalar accessors - get_samplers(): Simplified to use scalar sampler field 4. Updated Parameter File Writing (src/ert/run_models/_create_run_path.py): - Added special handling for EverestControl objects - Groups controls by output_file - Aggregates their values and writes a single JSON file per group - Preserves the nested JSON structure (e.g., {"x": 1.5, "y": 2.5}) 5. Updated All Usages: - everest_run_model.py: Flattens the list of lists returned by to_ert_parameter_config() - All test files: Updated to use the new flattened structure Benefits: - Consistency with ERT: Each EverestControl now maps to one scalar value, making it consistent with other ERT parameter types like GenKwConfig - Simpler Structure: The refactored code is more straightforward - no nested loops over controls and their variables - Easier to Extend: Makes equinor#12647 easier to solve by having a cleaner 1:1 mapping - Backward Compatible: All existing tests pass without changes to user-facing configuration Test Results: - ✅ All test_everest_control.py tests pass (5/5) - ✅ All test_controls.py tests pass (17/17) - ✅ All test_ropt_initialization.py tests pass (14/14) The refactoring is complete and ready for review!
yngve-sk
force-pushed
the
26.02.unroll-everestcontrols
branch
from
3355627 to
641dc22
Compare
It is "batched" similarly to genkws in _create_run_path
| continue | ||
|
|
||
| # Write aggregated EverestControl JSON files | ||
| start_time = time.perf_counter() |
There was a problem hiding this comment.
note2self/reviewer : This part ensures everest controls stay written "aggregated" even though they are specified as configs one-by-one. This can be removed once all everest forward models depending on the aggregated structure are refactored to use the flattened structure.
yngve-sk
force-pushed
the
26.02.unroll-everestcontrols
branch
from
ac7bf7a to
d74bd89
Compare
| if isinstance(param, EverestControl): | ||
| # Collect EverestControls for later aggregated writing | ||
| everest_controls_by_file[param.output_file].append(param) | ||
| export_timings[param.type] += time.perf_counter() - start_time |
There was a problem hiding this comment.
This timing is probably meaningless, it takes zero time to add it to the dict
| else: | ||
| data[key_without_group] = value | ||
|
|
||
| file_path.write_text(json.dumps(data), encoding="utf-8") |
There was a problem hiding this comment.
should add indent=2 to dumps for easier human reading of the files on disk.
control group is never None for EverestControl
|
|
||
| if param.name in scalar_data: | ||
| if isinstance(param, EverestControl): | ||
| # Collect EverestControls for later aggregated writing |
| formatted_names = self.formatted_control_names | ||
| formatted_names_dotdash = self.formatted_control_names_dotdash | ||
|
|
||
| idx = 0 |
There was a problem hiding this comment.
Can use for variable, idx in enumerate(self.variables) instead.
| sampler=variable.sampler or self.sampler, | ||
| ) | ||
| controls.append(control) | ||
| idx += 1 |
There was a problem hiding this comment.
remove this if enumerate is used
There was a problem hiding this comment.
The counter goes over the nested list and must match with formatted_control_names_dotdash. Could leave it for later to refactor these out and switch to enumerate though, suggest separate issue
| scaled_ranges=[(-1.0, 1.0), (-1.0, 1.0), (-1.0, 1.0)], | ||
| samplers=[None, None, None], | ||
| ) | ||
| # Create three separate controls (refactored structure) |
| ) | ||
| # Create three separate controls (refactored structure) | ||
| controls = [ | ||
| EverestControl( |
There was a problem hiding this comment.
can use some comprehension here to save lines
| mock_ensemble.load_parameters.return_value = mock_df | ||
|
|
||
| run_path = tmp_path / "runpath" / "realization-5" | ||
| # Create separate DataFrames for each control |
| run_path = tmp_path / "runpath" / "realization-5" | ||
|
|
||
| mock_ensemble.load_parameters.assert_called_once_with("point", 5) | ||
| # Simulate the aggregated file writing from _generate_parameter_files |
| ) | ||
| # Create three separate controls with nested keys | ||
| # The input_key will be like "point.x.0" which after removing "point." becomes "x.0" | ||
| # This creates JSON structure {"x": {"0": 1.5, "1": 2.5, "2": 3.5}} |
There was a problem hiding this comment.
replace comment with a comprehension
| ) | ||
| mock_ensemble.load_parameters.return_value = mock_df | ||
|
|
||
| # Create separate DataFrames for each control |
| run_path = tmp_path / "runpath" / "realization-5" | ||
|
|
||
| mock_ensemble.load_parameters.assert_called_once_with("point", 5) | ||
| # Simulate the aggregated file writing from _generate_parameter_files |
There was a problem hiding this comment.
should we not use _generate_parameter_files instead of replicating it here???
| control.write_to_runpath(run_path, 5, mock_ensemble) | ||
| run_path.mkdir(parents=True) | ||
|
|
||
| # Simulate the aggregated file writing from _generate_parameter_files |
There was a problem hiding this comment.
why simulate instead of actually test it?
| control = EverestControl( | ||
| name="point", | ||
| input_keys=["point.x", "point.y", "point.z"], | ||
| name="point.x", |
There was a problem hiding this comment.
agree that it seems pointless to include y and z for the purpose of this test.
| } | ||
|
|
||
| other_param = {"type": "gen_kw", "name": "P1", "distribution": {"name": "uniform"}} | ||
|
|
There was a problem hiding this comment.
Do not need an empty line between each statement. Maybe only around migrage(root).
berland
left a comment
berland
left a comment
There was a problem hiding this comment.
Think this looks good! Time to squash and write a good commit message.
yngve-sk
added
the
release-notes:skip
3 participants
Issue
Resolves #12853
Approach
Short description of the approach
(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'just rapid-tests')When applicable