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.

Sign up for GitHub

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

Jump to bottom

Add designmatrix for design2params #8715

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

Add designmatrix for design2params #8715

wants to merge 11 commits into from

Conversation

larsevj
Copy link
Contributor

@larsevj larsevj commented Sep 16, 2024 •
edited
Loading

Issue
Resolves

Approach
Short description of the approach

(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/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

@larsevj larsevj force-pushed the add_designmatrix_for_design2params branch 7 times, most recently from 619acb5 to ce827c6 Compare September 19, 2024 13:21
@larsevj
Copy link
Contributor Author

larsevj commented Sep 19, 2024

Some remaining questions/issues from this pr:

  • Need to figure out how we want to handle already existing gen_kw parameters, with the same name when creating an experiment from the design matrix. Design2params seems to prefer the already existent parameters in parameters.txt and drops the values from the designmatrix in this case. Note that design2params has no notion of parameter groups and so it will not necessarily detect duplicate param names, which will then be handled by design_kw.
  • Design2params also produces designmatrix.txt and designparameters.txt in the runpath.
  • Discuss how to combine initialization of parameters from design matrix and current run models.
  • Add more validation and tests. For instance what happens if you input dates, or other types. Currently everything is parsed as strings and then converted to floats if possible.

@larsevj larsevj force-pushed the add_designmatrix_for_design2params branch from ce827c6 to b2f859e Compare September 19, 2024 15:07
oyvindeide
oyvindeide reviewed Sep 24, 2024
View reviewed changes
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

Looks like a good start! Have some comments that might be good to take into account before merging.

src/ert/sensitivity_analysis/design_matrix.py
return design_matrix_sheet


def initialize_parameters(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest moving this code around a bit, so it is more in line with the sample_prior function.

tests/unit_tests/sensitivity_analysis/test_design_matrix.py Outdated
@pytest.mark.usefixtures("copy_poly_case")
def test_design_matrix(copy_poly_case):
config_text = """
QUEUE_SYSTEM LOCAL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest just appending DESIGN_MATRIX design_matrix.xlsx to the regular poly.ert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

test-data/poly_example/poly.ert Outdated
@@ -6,6 +6,7 @@ RUNPATH poly_out/realization-<IENS>/iter-<ITER>
OBS_CONFIG observations
REALIZATION_MEMORY 50mb

DESIGN_MATRIX design_matrix.xlsx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest removing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

tests/unit_tests/sensitivity_analysis/test_design_matrix.py Outdated
fhandle.write(config_text)
ert_config = ErtConfig.from_file("my_config.ert")
with open_storage(ert_config.ens_path, mode="w") as ert_storage:
design_frame = read_design_matrix(ert_config, "design_matrix.xlsx")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest creating a small dataframe here with test data, and exporting that to xlsx, which will make creating asserts easier

src/ert/sensitivity_analysis/design_matrix.py
DESIGN_MATRIX_GROUP = "DESIGN_MATRIX"


def read_design_matrix(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could probably have some unit tests for this.

src/ert/sensitivity_analysis/design_matrix.py
if designsheetname == defaultssheetname:
raise ValueError("Design-sheet and defaults-sheet can not be the same")

# This should probably handle/(fill in) missing values in design_matrix_sheet as well
Copy link
Collaborator

Choose a reason for hiding this comment

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

When merging suggest adding this as an issue instead (which is fine, dont have to fix it in this PR). Same goes for the other comments

src/ert/gui/tools/manage_experiments/manage_experiments_panel.py
@@ -142,3 +197,69 @@ def eventFilter(self, a0: Optional[QObject], a1: Optional[QEvent]) -> bool:
if a1 is not None and a1.type() == QEvent.Type.Close:
self.notifier.emitErtChange()
return super().eventFilter(a0, a1)

def _add_initialize_from_design_matrix_tab(self) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We were talking about doing this with Ensemble experiment instead, did you check how much work that would take?

@larsevj larsevj force-pushed the add_designmatrix_for_design2params branch 3 times, most recently from bd9e22e to 9fdf08b Compare September 24, 2024 15:40
@xjules
Copy link
Contributor

xjules commented Sep 25, 2024 •
edited
Loading

@larsevj as discussed yesterday, let's split this PR into smaller ones; ie. I'd start by introducing DESIGN_MATRIX keyword.

@larsevj larsevj force-pushed the add_designmatrix_for_design2params branch from 9fdf08b to d6a78a0 Compare September 25, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oyvindeide oyvindeide oyvindeide left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
christmas-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@larsevj @xjules @oyvindeide @eivindjahren