Add design matrix panel for parameters preview #8910

xjules · 2024-10-08T13:39:05Z

Issue
Resolves #8825

NB! This PR is based on #8800
The relevant files are:
-- src/ert/gui/simulation/ensemble_experiment_panel.py
-- src/ert/gui/simulation/experiment_panel.py
-- src/ert/gui/tools/design_matrix/design_matrix_panel.py
-- tests/ert/unit_tests/gui/simulation/test_run_dialog.py

Merging is blocked as it requires the PR above to be merged.

Now it's rebased

Approach
Add a QDialog that shows pd.DataFrame as QTableView

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

codecov-commenter · 2024-10-09T07:34:05Z

Codecov Report

Attention: Patch coverage is 45.45455% with 30 lines in your changes missing coverage. Please review.

Project coverage is 91.59%. Comparing base (072decc) to head (a2a8018).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...ert/gui/tools/design_matrix/design_matrix_panel.py	27.58%	21 Missing ⚠️
...rc/ert/gui/simulation/ensemble_experiment_panel.py	64.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8910      +/-   ##
==========================================
- Coverage   91.70%   91.59%   -0.11%     
==========================================
  Files         343      344       +1     
  Lines       21235    21294      +59     
==========================================
+ Hits        19473    19504      +31     
- Misses       1762     1790      +28

Flag	Coverage Δ
cli-tests	`39.73% <0.00%> (-0.04%)`	⬇️
gui-tests	`73.55% <30.90%> (-0.12%)`	⬇️
performance-tests	`50.29% <25.45%> (-0.05%)`	⬇️
unit-tests	`80.33% <45.45%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xjules · 2024-10-09T08:25:40Z

Preview screenshot:

jonathan-eq · 2024-10-11T10:02:32Z

src/ert/gui/simulation/ensemble_experiment_panel.py

-    def __init__(self, ensemble_size: int, run_path: str, notifier: ErtNotifier):
+    def __init__(
+        self,
+        analysis_config: AnalysisConfig,


Do we need the entire analysisconfig, or only designmatrix?

Good question and I have thought about it and decided on using the entire config:

other panels get the entire analysisconfig too and thus it makes the api more uniform

when doing the validation ( not there yet), we might to access more of the configs

jonathan-eq · 2024-10-11T10:03:27Z

src/ert/gui/simulation/ensemble_experiment_panel.py

@@ -78,6 +83,23 @@ def __init__(self, ensemble_size: int, run_path: str, notifier: ErtNotifier):
        )
        layout.addRow("Active realizations", self._active_realizations_field)

+        design_matrix = analysis_config.design_matrix
+        dm_param_button = QPushButton("Show parameters")


Can we rename dm_param_button to show_dm_param_button?

jonathan-eq · 2024-10-11T10:07:38Z

src/ert/gui/simulation/ensemble_experiment_panel.py

+        if design_matrix is not None:
+            if design_matrix.design_matrix_df is None:
+                design_matrix.read_design_matrix()
+            if (
+                design_matrix.design_matrix_df is not None
+                and not design_matrix.design_matrix_df.empty
+            ):
+                viewer = DesignMatrixPanel(design_matrix.design_matrix_df)
+                viewer.setMinimumHeight(500)
+                viewer.setMinimumWidth(1000)
+                viewer.adjustSize()
+                viewer.exec_()
+


Can we add

if design_matrix is None: return

and remove the indentation?

jonathan-eq · 2024-10-11T10:09:18Z

src/ert/gui/tools/design_matrix/design_matrix_panel.py

+    ) -> None:
+        super().__init__(parent)
+
+        self.setWindowTitle("Design matrix parameters viewer")


Should we have the title as viewer or preview?

What do you mean by title?

Can we change:
self.setWindowTitle("Design matrix parameters viewer") to
self.setWindowTitle("Design matrix parameters preview")

Right, from this it sounds you are fine with both 😄

Should we have the title as viewer or preview?

Yes, we can keep it as viewer 👌

Just change it to: f"Design matrix parameters from {filename}"

Even better :)

jonathan-eq · 2024-10-11T10:15:48Z

tests/ert/unit_tests/gui/simulation/test_run_dialog.py

+    "design_matrix_entry",
+    (True, False),
+)
+def test_that_design_matrix_show_parameters_button_is_enabled(


Here we are also testing that the button should be disabled, but I cannot think of a better name.
Maybe we can add to the test that when you push the button, the design matrix param viewer should pop up?

Hm, like in another test or inside this one?

We should really keep the unit tests at a unit level, but here I would test it in the same one. Then we would get to test the button visibility and the click functionality. We could call the test test_design_matrix_show_parameters_button

I'd need to create a pandas frame and it'd become integration I think. But maybe it's still fine to have as unit though 🤷

Ah, you are right. Let's keep it as is.

Ah, you are right. Let's keep it as is.

I'll give it a go 😸

jonathan-eq

Looks very good! See minor nitpicking 🚠

larsevj · 2024-10-11T10:21:56Z

src/ert/gui/simulation/ensemble_experiment_panel.py

+        button_layout.addStretch()  # Add stretch to push the button to the left
+
+        layout.addRow("Design Matrix", button_layout)
+        if design_matrix is not None:


I propose to only make the button if design_matrix exist, then it will be less confusion what this new but disabled thing is. And so only people that have actually set DESIGN_MATRIX will see this option at all.
But can of course be up for discussion.

Hm, I made it on purpose mainly due to testing. But could be done though.

larsevj · 2024-10-11T10:25:29Z

src/ert/gui/tools/design_matrix/design_matrix_panel.py

+    ) -> None:
+        super().__init__(parent)
+
+        self.setWindowTitle("Design matrix parameters viewer")


I would propose to include the xlsx filename in the title, for instance Parameters from some_file.xlsx.

jonathan-eq · 2024-10-11T12:28:24Z

tests/ert/unit_tests/gui/simulation/test_run_dialog.py

+            with patch.object(
+                DesignMatrix, "design_matrix_df", design_matrix_df
+            ), patch.object(DesignMatrix, "xls_filename", Path(xls_filename)):
+                qtbot.mouseClick(show_dm_parameters, Qt.LeftButton)


Can we also assert the new component is showing?

I have removed it

- Button to show the parameters is shown when design_matrix is present - Add test for design matrix show parameters button

jonathan-eq

LGTM!

xjules self-assigned this Oct 8, 2024

xjules force-pushed the dm_panel branch 3 times, most recently from b47bd57 to 25111b6 Compare October 9, 2024 07:17

xjules force-pushed the dm_panel branch 2 times, most recently from 65b1a97 to 372aaab Compare October 9, 2024 08:21

xjules marked this pull request as ready for review October 9, 2024 09:54

xjules force-pushed the dm_panel branch from 3b336a4 to 7f09561 Compare October 9, 2024 11:35

jonathan-eq self-requested a review October 10, 2024 06:21

xjules mentioned this pull request Oct 10, 2024

Run ensemble experiment with design matrix #8941

Merged

11 tasks

xjules force-pushed the dm_panel branch from 0a58473 to 313e578 Compare October 11, 2024 09:46

jonathan-eq reviewed Oct 11, 2024

View reviewed changes

jonathan-eq requested changes Oct 11, 2024

View reviewed changes

larsevj reviewed Oct 11, 2024

View reviewed changes

jonathan-eq reviewed Oct 11, 2024

View reviewed changes

xjules force-pushed the dm_panel branch from 07aaf7c to ebff633 Compare October 11, 2024 13:12

Add DesignMatrixPanel to show DataFrame parameters in a table

a2a8018

- Button to show the parameters is shown when design_matrix is present - Add test for design matrix show parameters button

xjules force-pushed the dm_panel branch from ebff633 to a2a8018 Compare October 11, 2024 13:25

jonathan-eq approved these changes Oct 11, 2024

View reviewed changes

xjules added release-notes:new-feature Automatically categorise as new feature in release notes sensitivity labels Oct 11, 2024

xjules enabled auto-merge (rebase) October 11, 2024 13:34

xjules merged commit 391a25f into equinor:main Oct 11, 2024
56 checks passed

xjules deleted the dm_panel branch October 11, 2024 14:32

Add design matrix panel for parameters preview #8910

Add design matrix panel for parameters preview #8910

Uh oh!

Conversation

xjules commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

When applicable

Uh oh!

codecov-commenter commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xjules commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xjules Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xjules Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-eq left a comment

Choose a reason for hiding this comment

Uh oh!

larsevj Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xjules Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathan-eq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xjules commented Oct 8, 2024 •

edited

Loading

codecov-commenter commented Oct 9, 2024 •

edited

Loading

xjules commented Oct 9, 2024 •

edited

Loading

xjules Oct 11, 2024 •

edited

Loading

xjules Oct 11, 2024 •

edited

Loading

larsevj Oct 11, 2024 •

edited

Loading

xjules Oct 11, 2024 •

edited

Loading