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

Have DESIGN_MATRIX not require design_sheet and default_sheet #10272

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Have DESIGN_MATRIX not require design_sheet and default_sheet #10272

wants to merge 2 commits into from
+63 −81

Conversation

jonathan-eq
Copy link
Contributor

@jonathan-eq jonathan-eq commented Mar 11, 2025 •
edited
Loading

Issue
Resolves #10270

Approach
This commit makes the DESIGN_MATRIX keyword not require arguments for design and default sheet. Now DESIGN_SHEET defaults to DesignSheet and DEFAULT_SHEET defaults to "DefaultSheet"

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

@jonathan-eq jonathan-eq added the release-notes:improvement Automatically categorise as improvement in release notes label Mar 11, 2025
@jonathan-eq jonathan-eq force-pushed the design_matrix branch 2 times, most recently from 7290ae6 to eb2b139 Compare March 11, 2025 14:27
@jonathan-eq jonathan-eq marked this pull request as ready for review March 11, 2025 14:28
@jonathan-eq jonathan-eq force-pushed the design_matrix branch 2 times, most recently from e41d3b7 to 7424ebd Compare March 11, 2025 14:39
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Mar 11, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #10272 will degrade performances by 10.54%

Comparing jonathan-eq:design_matrix (6ae9dc3) with main (4014637)

Summary

❌ 1 regressions
✅ 24 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
❌ test_direct_dark_performance[gen_x: 20, sum_x: 20 reals: 10-gen_data_with_obs-get_parameters] 252.1 µs 281.8 µs -10.54%

@larsevj
Copy link
Collaborator

larsevj commented Mar 12, 2025

Would not DesignSheet be a more natural default than DesignSheet01 ?

@jonathan-eq
Copy link
Contributor Author

Would not DesignSheet be a more natural default than DesignSheet01 ?

Maybe, but that was just the example in the issue. What do you think @JHolba @xjules ?

larsevj
larsevj reviewed Mar 12, 2025
View reviewed changes
tests/ert/unit_tests/config/test_analysis_config.py Outdated
def test_invalid_design_matrix_format_raises_validation_error():
with pytest.raises(
ConfigValidationError,
match="DESIGN_MATRIX must be of format \\.xls or \\.xlsx; is 'my_matrix\\.txt'",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this test removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, it was probably caught in the crossfire 🤔

@jonathan-eq
Copy link
Contributor Author

Would not DesignSheet be a more natural default than DesignSheet01 ?

This is also the current default in design2params, but there the defaultsheet default is "DefaultValues"

@sondreso
Copy link
Collaborator

Would not DesignSheet be a more natural default than DesignSheet01 ?

This is also the current default in design2params, but there the defaultsheet default is "DefaultValues"

We should have the same defaults as DESIGN2PARAMS here, otherwise we might cause unnecessary confusion when transitioning users over 🙂

@jonathan-eq jonathan-eq force-pushed the design_matrix branch 2 times, most recently from dffc7d4 to 7526c15 Compare March 13, 2025 06:38
@larsevj
Copy link
Collaborator

larsevj commented Mar 13, 2025

Would not DesignSheet be a more natural default than DesignSheet01 ?

This is also the current default in design2params, but there the defaultsheet default is "DefaultValues"

If I try to run without arguments for design_sheet and default_sheet I get the following error from design2params:

File /data/design_matrix.xlsx is probably not of correct type. Failed with exception 'Worksheet named '<designsheet>' not found'

@larsevj
Copy link
Collaborator

larsevj commented Mar 13, 2025 •
edited
Loading

I suspect the actual default values today are <designsheet> and <defaultssheet> ..

Which hopefully no one uses.

@jonathan-eq
Copy link
Contributor Author

<designsheet

I am not sure, that seems to be the values expected to be substituted

@jonathan-eq jonathan-eq force-pushed the design_matrix branch 2 times, most recently from a91b17f to 9683760 Compare March 18, 2025 06:57
xjules
xjules reviewed Mar 18, 2025
View reviewed changes
tests/ert/ui_tests/cli/analysis/test_design_matrix.py Outdated
@@ -54,7 +54,7 @@ def test_run_poly_example_with_design_matrix():
NUM_REALIZATIONS 10
MIN_REALIZATIONS 1
GEN_DATA POLY_RES RESULT_FILE:poly.out
DESIGN_MATRIX poly_design.xlsx DESIGN_SHEET:DesignSheet01 DEFAULT_SHEET:DefaultSheet
DESIGN_MATRIX poly_design.xlsx DESIGN_SHEET:DesignSheet DEFAULT_SHEET:DefaultSheet
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe now you can remove all those params?

@jonathan-eq jonathan-eq force-pushed the design_matrix branch 2 times, most recently from c933fa1 to 1a54a5b Compare March 19, 2025 10:57
@jonathan-eq jonathan-eq requested a review from xjules March 19, 2025 10:58
@jonathan-eq
Have DESIGN_MATRIX not require design_sheet and default_sheet
88cbaec 
This commit makes the DESIGN_MATRIX keyword not require arguments for design and default
sheet. Now DESIGN_SHEET defaults to `DesignSheet` and `DEFAULT_SHEET` defaults to "DefaultSheet".
This is the same defaults that are used in design2params.
larsevj
larsevj reviewed Mar 19, 2025
View reviewed changes
tests/ert/ui_tests/cli/analysis/test_design_matrix.py
@@ -358,7 +358,7 @@ def test_design_matrix_on_esmda(experiment_mode, ensemble_name, iterations):
GEN_KW COEFFS_B coeff_priors_b
GEN_KW COEFFS_C coeff_priors_c
GEN_DATA POLY_RES RESULT_FILE:poly.out
DESIGN_MATRIX design_matrix.xlsx DESIGN_SHEET:DesignSheet01 DEFAULT_SHEET:DefaultSheet
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have any tests left at all that do not use the default values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but maybe we should 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xjules xjules xjules left review comments

@larsevj larsevj larsevj left review comments

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

Assignees
No one assigned
Labels
release-notes:improvement Automatically categorise as improvement in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make design and default sheet not required arguments for the DESIGN_MATRIX keyword
4 participants
@jonathan-eq @larsevj @sondreso @xjules