Refactor BaseRunModel #9676
Refactor BaseRunModel #9676
Conversation
frode-aarstad
added
the
release-notes:maintenance
frode-aarstad
force-pushed
the
refactor-baserunmodel
branch
2 times, most recently
from
95a4211 to
a049423
Compare
CodSpeed Performance ReportMerging #9676 will improve performances by 10.99%Comparing Summary
Benchmarks breakdown
|
frode-aarstad
force-pushed
the
refactor-baserunmodel
branch
from
a049423 to
4b764b8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9676 +/- ##
==========================================
- Coverage 91.74% 91.73% -0.01%
==========================================
Files 426 426
Lines 26518 26553 +35
==========================================
+ Hits 24328 24359 +31
- Misses 2190 2194 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/ert/run_models/base_run_model.py
Outdated
| @@ -146,27 +159,37 @@ def __init__( | |||
| the forward model and passing events back through the supplied queue. | |||
| """ | |||
| self._total_iterations = total_iterations | |||
| config.analysis_config.num_iterations = total_iterations | |||
| # config.analysis_config.num_iterations = total_iterations | |||
There was a problem hiding this comment.
Stray comment? Not sure exactly why this was there in the first place also... Found some logic using it in the ExportRunpathJob, so maybe we still need to mutate it? Could be done from model_factory.py in _setup_iterative_ensemble_smoother to preserve old behavior
src/ert/run_models/base_run_model.py
Outdated
| @@ -146,27 +159,37 @@ def __init__( | |||
| the forward model and passing events back through the supplied queue. | |||
| """ | |||
| self._total_iterations = total_iterations | |||
| config.analysis_config.num_iterations = total_iterations | |||
| # config.analysis_config.num_iterations = total_iterations | |||
| active_realizations=[], # Set dynamically in run_forward_model() | ||
| ) | ||
| self.support_restart = False | ||
| self._parameter_configuration = config.ensemble_config.parameter_configuration | ||
| self._parameter_configs = config.ensemble_config.parameter_configs |
There was a problem hiding this comment.
Do we need to keep both these fields? One is a dict, other is a list, could just keep the dict and do .values() or something like that. Also, the second invocation:
ext_config = self.ert_config.ensemble_config.parameter_configs[control_name]
Here the experiment will always exist, so we could do self._experiment.parameter_configuration to get the parameter configs, and extract the ExtParam ones (should only be those anyway).
There was a problem hiding this comment.
This should probably be part of a refactor of EnsembleConfig. Both parameter_config and parameter_configuration seems to be used quite a bit around in the code.
frode-aarstad
force-pushed
the
refactor-baserunmodel
branch
3 times, most recently
from
d126be3 to
200b926
Compare
|
Are you sure about the removal of this part: |
You are right. Its probably best to leave it as it was |
I think we have to move the "edit analysis config num iterations" behavior to the ies part of model_factory as outlined in my previous comment to preserve the behavior and also not have the entire AnalysisConfig in the BaseRunModel init signature |
frode-aarstad
force-pushed
the
refactor-baserunmodel
branch
from
200b926 to
64aab1f
Compare
frode-aarstad
force-pushed
the
refactor-baserunmodel
branch
from
64aab1f to
6b7f276
Compare
Issue
Resolves #9604
Approach
Short description of the approach
(Screenshot of new behavior in GUI if applicable)
git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')When applicable