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

Add ScalarParameters replacing GenKW in parameter config #10095

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

xjules
Copy link
Contributor

@xjules xjules commented Feb 18, 2025

Issue
Resolves #10088

Approach
- transformations become dataclases -> TODO: pydantic
- ScalarParameter represents a single parameter instance with a
datasource (DESIGN_MATRIX, SAMPLED)
- all ScalarParameter instances are bound by
ScalarParameters(ParameterConfig) and are stored for all realizations
in a single pl.Dataframe (parquet) file.
- API to load / save scalar parameters is in local_ensemble load|save_param_scalar
- DESIGN_MATRIX now creates only ScalarParameters instances and
modifies the ScalarParameterc Config.

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

@xjules xjules self-assigned this Feb 18, 2025
@xjules xjules added the release-notes:refactor PR changes code without changing ANY (!) behavior. label Feb 18, 2025
Copy link

codspeed-hq bot commented Feb 18, 2025

CodSpeed Performance Report

Merging #10095 will not alter performance

Comparing xjules:draft_paramcfg (33ef660) with main (cd27051)

Summary

✅ 25 untouched benchmarks



@dataclass
class TransUnifSettings:
Copy link
Contributor Author

@xjules xjules Feb 24, 2025

Choose a reason for hiding this comment

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

TODO: All distributions should contain LOG10 property
This will be handled directly by the type of distribution.

if group_name not in transformed_dict:
transformed_dict[group_name] = {}
transformed_dict[group_name][param_name] = transformed_value
# todo log handling when distribution requires it
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: here is also the place that we need to handle template_file / output_file!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First draft of the treatment of template / output file is in place.

]
)

def save_parameters(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: redesign API for save_parameters?

@xjules xjules force-pushed the draft_paramcfg branch 2 times, most recently from 096cbc4 to ebef6f6 Compare March 4, 2025 09:44
@xjules xjules changed the title Add draft for parameter config Add ScalarParameters replacing GenKW in parameter config Mar 7, 2025
)
)
# TODO refactor this test
# @pytest.mark.parametrize(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Get rid of most of it

Copy link
Contributor

Choose a reason for hiding this comment

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

The best tests are the ones that are removed :)

@xjules xjules force-pushed the draft_paramcfg branch 2 times, most recently from 80bc01b to ddcf1e0 Compare March 8, 2025 18:30
 - transformations become dataclases -> TODO: pydantic
 - ScalarParameter represents a single parameter instance with a
   datasource (DESIGN_MATRIX, SAMPLED)
 - all ScalarParameter instances are bound by
   ScalarParameters(ParameterConfig) and are stored for all realizations
in a single pl.Dataframe (parquet) file.
 - API to load / save scalar parameters is in local_ensemble load|save_param_scalar
 - DESIGN_MATRIX now creates only ScalarParameters instances and
   modifies the ScalarParameterc Config.
 - Reimplement load_all_gen_kwn_data
@codecov-commenter
Copy link

❌ 13 Tests Failed:

Tests completed Failed Passed Skipped
440 13 427 24
View the top 3 failed test(s) by shortest run time
tests/ert/ui_tests/gui/test_csv_export.py::test_csv_export[*]
Stack Traces | 0.002s run time
item = <Function test_csv_export[*]>

    @pytest.hookimpl(wrapper=True, tryfirst=True)
    def pytest_runtest_setup(item):
        """
        Hook called after before test setup starts, to start capturing exceptions
        as early as possible.
        """
        capture_enabled = _is_exception_capture_enabled(item)
        if capture_enabled:
            item.qt_exception_capture_manager = _QtExceptionCaptureManager()
            item.qt_exception_capture_manager.start()
>       result = yield

.venv/lib/python3.12.../site-packages/pytestqt/plugin.py:180: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.../ui_tests/gui/conftest.py:120: in _esmda_run
    log_paths = list(Path(config.analysis_config.log_path).iterdir())
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = PosixPath('.../pytest-0/test-data0/update_log')

    def iterdir(self):
        """Yield path objects of the directory contents.
    
        The children are yielded in arbitrary order, and the
        special entries '.' and '..' are not included.
        """
>       for name in os.listdir(self):
E       FileNotFoundError: [Errno 2] No such file or directory: '.../pytest-0/test-data0/update_log'

.../hostedtoolcache/Python/3.12.9.../x64/lib/python3.12/pathlib.py:1056: FileNotFoundError
tests/ert/ui_tests/gui/test_main_window.py::test_that_load_results_manually_can_be_run_after_esmda
Stack Traces | 0.002s run time
item = <Function test_that_load_results_manually_can_be_run_after_esmda>

    @pytest.hookimpl(wrapper=True, tryfirst=True)
    def pytest_runtest_setup(item):
        """
        Hook called after before test setup starts, to start capturing exceptions
        as early as possible.
        """
        capture_enabled = _is_exception_capture_enabled(item)
        if capture_enabled:
            item.qt_exception_capture_manager = _QtExceptionCaptureManager()
            item.qt_exception_capture_manager.start()
>       result = yield

.../ert/ert/.venv/lib/python3.12.../site-packages/pytestqt/plugin.py:180: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.../ui_tests/gui/conftest.py:120: in _esmda_run
    log_paths = list(Path(config.analysis_config.log_path).iterdir())
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = PosixPath('.../pytest-0/test-data4/update_log')

    def iterdir(self):
        """Yield path objects of the directory contents.
    
        The children are yielded in arbitrary order, and the
        special entries '.' and '..' are not included.
        """
>       for name in os.listdir(self):
E       FileNotFoundError: [Errno 2] No such file or directory: '.../pytest-0/test-data4/update_log'

.../hostedtoolcache/Python/3.12.9.../x64/lib/python3.12/pathlib.py:1056: FileNotFoundError
tests/ert/ui_tests/gui/test_main_window.py::test_that_the_manage_experiments_tool_can_be_used
Stack Traces | 0.002s run time
item = <Function test_that_the_manage_experiments_tool_can_be_used>

    @pytest.hookimpl(wrapper=True, tryfirst=True)
    def pytest_runtest_setup(item):
        """
        Hook called after before test setup starts, to start capturing exceptions
        as early as possible.
        """
        capture_enabled = _is_exception_capture_enabled(item)
        if capture_enabled:
            item.qt_exception_capture_manager = _QtExceptionCaptureManager()
            item.qt_exception_capture_manager.start()
>       result = yield

.../ert/ert/.venv/lib/python3.12.../site-packages/pytestqt/plugin.py:180: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.../ui_tests/gui/conftest.py:120: in _esmda_run
    log_paths = list(Path(config.analysis_config.log_path).iterdir())
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = PosixPath('.../pytest-0/test-data4/update_log')

    def iterdir(self):
        """Yield path objects of the directory contents.
    
        The children are yielded in arbitrary order, and the
        special entries '.' and '..' are not included.
        """
>       for name in os.listdir(self):
E       FileNotFoundError: [Errno 2] No such file or directory: '.../pytest-0/test-data4/update_log'

.../hostedtoolcache/Python/3.12.9.../x64/lib/python3.12/pathlib.py:1056: FileNotFoundError

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

Copy link
Contributor

@jonathan-eq jonathan-eq left a comment

Choose a reason for hiding this comment

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

I don't think the tests are all that happy 🌧️

if isinstance(config_node, ScalarParameters):
return config_node.load_parameters_to_update(ensemble, iens_active_index)
dataset = config_node.load_parameters(ensemble, param_group, iens_active_index)
assert isinstance(dataset, np.ndarray), "dataset is not an numpy array"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we keeping this assertion?


if TYPE_CHECKING:
from ert.config import ParameterConfig
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can remove this TYPE_CHECKING

@@ -180,12 +182,12 @@ def hasNodeGenData(self, key: str) -> bool:
config = self.response_configs["gen_data"]
return key in config.keys

# TODO: This might not be needed but it retrieves the group names for genkw config
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this still needed?

)
)
# TODO refactor this test
# @pytest.mark.parametrize(
Copy link
Contributor

Choose a reason for hiding this comment

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

The best tests are the ones that are removed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:refactor PR changes code without changing ANY (!) behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Draft a new ParameterConfig given assumption that all parameter names are unique
3 participants