-
Notifications
You must be signed in to change notification settings - Fork 121
Create misfit export tab in Manage Experiments #12566
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
Create misfit export tab in Manage Experiments #12566
Conversation
9848742 to
f95a859
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12566 +/- ##
==========================================
- Coverage 90.57% 90.51% -0.06%
==========================================
Files 435 435
Lines 29914 30023 +109
==========================================
+ Hits 27095 27176 +81
- Misses 2819 2847 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
b7e0d2a to
9380f18
Compare
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new "Misfit" tab to the Manage Experiments dialog, allowing users to view and export misfit data alongside existing parameter data. The implementation refactors the existing ExportParametersDialog into a base ExportDialog class with two concrete implementations: ExportParametersDialog and ExportMisfitDialog.
Key changes:
- Extracted base ExportDialog class to share common export dialog functionality
- Implemented ExportMisfitDialog for exporting misfit data to CSV
- Added Misfit tab to the Manage Experiments UI with table view and export button
- Refactored tests to work with the new class hierarchy
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/ert/gui/tools/manage_experiments/export_parameters_dialog.py | Refactored to create base ExportDialog class and added ExportMisfitDialog subclass; enhanced file path validation |
| src/ert/gui/tools/manage_experiments/storage_info_widget.py | Added Misfit tab with table and export button; refactored frame creation into reusable helper method; implemented misfit data loading |
| tests/ert/ui_tests/gui/test_export_parameters_dialog.py | Refactored validation tests to use base ExportDialog class; split parametrized test into separate focused tests; added test for misfit export dialog |
Comments suppressed due to low confidence (1)
src/ert/gui/tools/manage_experiments/export_parameters_dialog.py:82
- The error message mentions "ensemble parameters" specifically, but this base class is now used for both parameters and misfit exports. The error message should be more generic (e.g., "Error exporting data") or the subclass should be able to customize the error message.
self._export_text_area.insertHtml(
"<span style='color: red;'>"
f"Error exporting ensemble parameters: {e!s}"
"</span><br>"
| df = LibresFacade.load_all_misfit_data(self._ensemble) | ||
| realization_column = pl.Series(df.index) | ||
| df = pl.from_pandas(df) | ||
| df.insert_column(0, realization_column) |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This data transformation logic (loading misfit data, extracting index as realization column, converting from pandas to polars, and inserting the column) is duplicated in storage_info_widget.py's get_misfit_df method (lines 446-452). Consider extracting this into a shared helper method or static method on ExportMisfitDialog to eliminate duplication and ensure consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a task for #12579
I could create a public method, but that would cause cirular imports. I could create a common endpoint somewhere, but that would be within the scope of the other issue.
| df = pl.from_pandas(df) | ||
| df.insert_column(0, realization_column) | ||
|
|
||
| df.write_csv(output_file) |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The misfit export doesn't specify float_precision when calling write_csv, while ExportParametersDialog uses float_precision=6 (line 152). For consistency with the CSV export workflow (csv_export.py line 147) and parameter exports, consider adding float_precision=6 here as well.
| df.write_csv(output_file) | |
| df.write_csv(output_file, float_precision=6) |
| def test_that_export_misfit_dialogue_exports_misfit_data_from_ensemble( | ||
| qtbot, snake_oil_storage: Storage | ||
| ): | ||
| ensemble = next(snake_oil_storage.ensembles) | ||
| dialog = ExportMisfitDialog(ensemble) | ||
|
|
||
| with patch.object( | ||
| pl.DataFrame, "write_csv", new_callable=MagicMock | ||
| ) as mock_write_csv: | ||
| dialog.export() | ||
|
|
||
| assert mock_write_csv.called |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test only verifies that write_csv was called but doesn't validate what data was actually exported. The test should verify that the exported misfit data matches the expected format and content - at minimum, check the DataFrame structure (columns, index) and ideally compare it against LibresFacade.load_all_misfit_data output. Consider following the pattern in test_that_export_writes_to_file_and_check_the_file_content which validates the actual exported file content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had trouble checking what the mock was called upon, I only got to extract the filename. I will have another go at this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have remade the solution, including a commit for passing DataFrames to the Export Classes instead of Ensembles, making testing a lot easier.
| if path.is_file(): | ||
| _set_invalid(tooltip_text=f"'{path!s}' is an existing file.") | ||
| return |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation prevents exporting to an existing file, which may be overly restrictive. Users typically expect to be able to overwrite files when exporting data. Consider either removing this check or prompting the user for confirmation instead of completely blocking the export. If this behavior is intentional, it should be tested to ensure it works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed this now.
| output_file: str = self._file_path_edit.text().strip() | ||
|
|
||
| df = LibresFacade.load_all_misfit_data(self._ensemble) | ||
| realization_column = pl.Series(df.index) |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When creating the realization_column Series from df.index, the Series doesn't preserve the index name "Realization" set in LibresFacade.load_all_misfit_data. Consider using pl.Series(name="Realization", values=df.index) or preserving the original index name with pl.Series(name=df.index.name, values=df.index) to maintain consistency with the source data structure.
| realization_column = pl.Series(df.index) | |
| realization_column = pl.Series(name=df.index.name, values=df.index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not True I believe
|
Seems like there is a bunch to improve here. Will get started with it =) |
2422596 to
4875143
Compare
| try: | ||
| output_file: str = self._file_path_edit.text().strip() | ||
| self._export_button.setEnabled(False) | ||
| parameters_df = self._ensemble.load_scalar_keys(transformed=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit will be generalized in a later commit
5da75e9 to
e5c78de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| def export_action(self) -> None: | ||
| raise NotImplementedError("Subclasses must implement this method.") | ||
|
|
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The export_action method is defined but never called anywhere in the codebase. Since ExportDialog directly performs exports via the export method and is instantiated directly (not subclassed), this method appears to be unused dead code and should be removed.
| def export_action(self) -> None: | |
| raise NotImplementedError("Subclasses must implement this method.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed!
e5c78de to
7fe4b58
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
src/ert/gui/tools/manage_experiments/export_dialog.py:117
- The validation logic treats "." as an invalid path (equivalent to empty string), but this is inconsistent. In Unix/Linux systems, "." represents the current directory and
Path(".")would returnTrueforis_dir(), so it should be caught by the directory check on line 119 instead. The special case for "." on line 115 is redundant and could be removed for consistency.
| "valid-export", | ||
| ], | ||
| ) | ||
| def test_that_file_path_validation_succeeds_on_valid_paths( |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name doesn't follow the specification style required by the coding guidelines. Test names should avoid vague terms like "succeeds". Instead, describe the specific behavior being verified.
Suggested rename: test_that_valid_file_paths_enable_export_button_and_clear_error_state
| from ert.gui.tools.manage_experiments.export_dialog import ExportDialog | ||
|
|
||
|
|
||
| def test_that_export_writes_to_file_and_check_the_file_content(qtbot, change_to_tmpdir): |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name doesn't follow the specification style required by the coding guidelines. Test names should start with test_that_ and clearly state the expected behavior without vague terms. The current name includes "and check" which is redundant and doesn't describe what correctness criterion is being tested.
Suggested rename: test_that_exported_csv_contains_the_provided_dataframe_values
| assert_invalidation_in_dialog(dialog, expected_error="No filename provided") | ||
|
|
||
|
|
||
| def test_that_file_path_validation_fails_on_non_existing_path(qtbot): |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name doesn't follow the specification style required by the coding guidelines. Test names should describe the expected behavior or invariant, not just state what fails. The phrase "validation fails" is a vague judgment rather than describing the concrete behavior being tested.
Suggested rename: test_that_non_existent_directory_path_shows_invalid_file_path_error
|
|
||
|
|
||
| @patch("polars.DataFrame.write_csv") | ||
| def test_that_export_failure_is_handled_correctly(patched_write_csv, qtbot): |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name doesn't follow the specification style required by the coding guidelines. Test names should avoid vague terms like "correctly" and "handled". Instead, describe the specific behavior being verified.
Suggested rename: test_that_export_shows_error_message_when_csv_write_raises_exception
| assert_invalidation_in_dialog(dialog, expected_error="Invalid file path") | ||
|
|
||
|
|
||
| def test_that_file_path_validation_fails_on_existing_directories(qtbot): |
Copilot
AI
Jan 7, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name doesn't follow the specification style required by the coding guidelines. Test names should describe the expected behavior or invariant, not just state what fails. The phrase "validation fails" is a vague judgment rather than describing the concrete behavior being tested.
Suggested rename: test_that_existing_directory_path_shows_directory_error_message
- Split up cases to easier know which states are valid and not - Improve test names to follow our CONTRIBUTING guidelines
b7eb95a to
c83a521
Compare
|
|
||
| def test_that_file_path_is_invalidated_given_empty_path(qtbot): | ||
| parameter_df = DataFrame({"a": 1, "b": 2}) | ||
| dialog = ExportDialog(parameter_df) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better for readability if you create an empty dataframe when the data in it is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I can change that!
frode-aarstad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good 🚀
By not needing the storage fixture anymore, the tests are reduced from taking 1 sec to 0.15 sec.
Extract creation of export table tab to class method
c73aa41 to
968f480
Compare
Issue
Resolves #12432
git rebase -i main --exec 'just rapid-tests')When applicable