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

Validate DESIGN_MATRIX when running DESIGN2PARAMS #10316

Merged
merged 1 commit into from
Mar 19, 2025
Merged

Validate DESIGN_MATRIX when running DESIGN2PARAMS #10316

merged 1 commit into from
Mar 19, 2025

Conversation

jonathan-eq
Copy link
Contributor

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

Issue
Resolves #10271

Approach
This commit makes us run validation for DESIGN_MATRIX when parsing configs using DESIGN2PARAMS, and log a warning if it would have failed.

(Screenshot of new behavior in GUI if applicable)

2025-03-14 09:02:28,837 - ert.config.ert_config - MainThread - WARNING - DESIGN_MATRIX validation of DESIGN2PARAMS would have failed with: Error reading design matrix dm.xlsx: Worksheet named 'default' not found
  • 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 marked this pull request as ready for review March 14, 2025 08:31
@jonathan-eq jonathan-eq added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Mar 14, 2025
@codspeed-hq CodSpeed HQ
Copy link

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

CodSpeed Performance Report

Merging #10316 will not alter performance

Comparing jonathan-eq:validate-ert-design-matrix (8ffd82d) with main (a0fda20)

Summary

✅ 25 untouched benchmarks

xjules
xjules reviewed Mar 14, 2025
View reviewed changes
src/ert/config/ert_config.py Outdated
if ert_design_matrix_error := ert_design_matrix._validate_design_matrix(
ert_design_matrix
):
raise Exception("\n".join(ert_design_matrix_error))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the full Exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it to ConfigurationError 👌

@xjules
Copy link
Contributor

xjules commented Mar 14, 2025

Could you write a test for this validation? It can be even part of test_design_matrix like test_design2params validation.

@jonathan-eq
Copy link
Contributor Author

Could you write a test for this validation? It can be even part of test_design_matrix like test_design2params validation.

I am not sure if we will have the DESIGN2PARAMS forward model installed when running our test suite..
I created a test but it fails with this ert.config.parsing.config_errors.ConfigValidationError: /Users/JONAK/Documents/FMU/SCOUT/ert/config.ert: Line 2 (Column 15-28): Could not find forward model step 'DESIGN2PARAMS' in list of installed forward model steps: []

@jonathan-eq jonathan-eq force-pushed the validate-ert-design-matrix branch 2 times, most recently from 0d364f6 to 6b4c554 Compare March 18, 2025 06:55
xjules
xjules reviewed Mar 18, 2025
View reviewed changes
tests/ert/unit_tests/config/test_ert_config.py Outdated
f"""\
NUM_REALIZATIONS 1
FORWARD_MODEL DESIGN2PARAMS(<xls_filename>={design_matrix_file}, <designsheet>=DesignSheet01,<defaultssheet>=DefaultSheet)
FORWARD_MODEL DESIGN2PARAMS(<xls_filename>={design_matrix_file}, <designsheet>=DesignSheet02,<defaultssheet>=DefaultSheet)
Copy link
Contributor

@xjules xjules Mar 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Where do you define DesignSheet02? I see now. But tbh I don't like this create_design_matrix "improvement" as it suggests that there might be more designsheets 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why wouldn't we accept multiple designsheets? I assumed that was something we supported because we had the 01 suffix in DesignSheet01.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right, we cannot have designsheet01 and designsheet02 in the same xlsx file..

@jonathan-eq jonathan-eq force-pushed the validate-ert-design-matrix branch from 6b4c554 to b3b8216 Compare March 18, 2025 10:45
xjules
xjules approved these changes Mar 18, 2025
View reviewed changes
Copy link
Contributor

@xjules xjules left a comment

Choose a reason for hiding this comment

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

I think it looks good 🚀

@jonathan-eq jonathan-eq requested a review from larsevj March 18, 2025 12:27
@jonathan-eq jonathan-eq force-pushed the validate-ert-design-matrix branch from b3b8216 to f23ce36 Compare March 18, 2025 13:21
@jonathan-eq
Validate DESIGN_MATRIX when running DESIGN2PARAMS
8ffd82d 
This commit makes us run validation for DESIGN_MATRIX when parsing
configs using DESIGN2PARAMS, and log a warning if it would have failed.
@jonathan-eq jonathan-eq force-pushed the validate-ert-design-matrix branch from f23ce36 to 8ffd82d Compare March 18, 2025 13:22
@jonathan-eq jonathan-eq merged commit 0f11d30 into equinor:main Mar 19, 2025
27 checks passed
@jonathan-eq jonathan-eq deleted the validate-ert-design-matrix branch March 19, 2025 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsevj larsevj larsevj left review comments

@xjules xjules xjules approved these changes

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

Successfully merging this pull request may close these issues.

Make use of DESIGN2PARAMS to run DESIGN_MATRIX validation
3 participants
@jonathan-eq @xjules @larsevj