Seismic cross plot by akustn · Pull Request #14266 · equinor/ert

akustn · 2026-08-24T14:15:03Z

Issue
Resolves #14200

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 'just rapid-tests')

When applicable

When screenshots are changed: Review screenshot-PR in ert-testdata,
merge screenshot-PR in ert-testdata before merging this PR.
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: Add backport label to latest release (format: 'backport release-branch-name')

codecov-commenter · 2026-08-24T14:21:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.95%. Comparing base (4ff378d) to head (f7d5f5f).
⚠️ Report is 40 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14266      +/-   ##
==========================================
+ Coverage   91.92%   91.95%   +0.02%     
==========================================
  Files         484      486       +2     
  Lines       33662    33792     +130     
==========================================
+ Hits        30944    31072     +128     
- Misses       2718     2720       +2

Flag	Coverage Δ
cli-tests	`36.40% <0.00%> (-0.15%)`	⬇️
fuzz	`44.29% <22.30%> (-0.09%)`	⬇️
gui-tests	`58.60% <43.84%> (-0.06%)`	⬇️
performance-and-unit-tests	`81.03% <100.00%> (+0.06%)`	⬆️
test	`45.89% <22.30%> (-0.10%)`	⬇️

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

Files with missing lines	Coverage Δ
src/ert/gui/plotting/ert_plots/__init__.py	`100.00% <100.00%> (ø)`
src/ert/gui/plotting/ert_plots/cross.py	`100.00% <100.00%> (ø)`
src/ert/gui/plotting/plot_api.py	`80.26% <100.00%> (+0.06%)`	⬆️
src/ert/gui/plotting/plot_window.py	`79.52% <100.00%> (+0.82%)`	⬆️
src/ert/gui/plotting/utils/plot_maps.py	`100.00% <100.00%> (ø)`
...tting/widgets/plot_realization_selection_widget.py	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

ertomatic · 2026-08-24T14:23:39Z

Screenshots differ from baselines. A baseline update PR has been prepared: equinor/ert-testdata#92

codspeed-hq · 2026-08-25T07:31:46Z

Merging this PR will not alter performance

✅ 36 untouched benchmarks

_{Comparing akustn:seismic-cross-plot (f7d5f5f) with main (4ff378d)}

achaikou

Good job with plots!! 🎉 And users seemed happy! 🐱

Unfortunately for you, I am a Very Evil Reviewer ™️ , so comments apply 😈

Btw, regarding the checkboxes:

PR name might be "Add seismic cross plot" as PR name is supposed to show up in the release notes (and would be default commit name I think if you squash)
Release-note label should be release-notes:unreleased-feature-changes if you think this is more seismic-related. release-notes:new-feature would have fit better if cross plot was enabled for other types, not only seismic. But could be used if you want too (then I think it will go to the top of release notes)
Commit history doesn't matter if you plan to squash, otherwise we can take it later!
Need to talk to someone about this message "Screenshots differ from the baseline". I think that PR should be checked and merged after yours is merged, but I never done this before so don't really know 😬

achaikou · 2026-08-26T13:55:57Z

            "Statistics",
            "Std dev",
            "Misfits",
+            "Cross plot",


Regarding commit name "fixup! Add seismic cross plot" in which this change is added:

I am not sure if you plan to squash and merge (then it doesn't matter), or rebase and merge (then this fixup change probably should refer to a different commit, as this is a test file).

"fixup! Add seismic cross plot" is a fixup for this specific commit, because locally I ran "just rapid-tests" which passed, but then discovered that by adding the cross plot there was a gui test failing, there for it is a fixup to the first commit.

And yes, I'll squash the fixups into the commits which they are a fixup for.

achaikou · 2026-08-26T14:11:55Z

+    numeric = pd.to_numeric(pd.Series(values), errors="coerce")
+    if numeric.notna().all():
+        return numeric.astype(np.float32).to_numpy()
+    return pd.Index(values).astype(str).to_numpy()


So this function may return numeric key or string key?
What if observations return numeric key and responses return string key?
Can those be matched?
And why do we need both, numbers and strings? What is the danger?

(And generally this function seems purely technical, with no domain logic behind it. It makes me wonder if we really need this function).

achaikou · 2026-08-26T14:38:34Z

+    responses = ensemble_data.copy()
+    responses.columns = _to_matchable_key(responses.columns.to_numpy())
+
+    common_keys = obs_by_key_index.index.intersection(responses.columns)


Everywhere we match observations and responses (in update's get_responses_and_observations and in EnsembleWidget), we now match with tolerance use_observation_locations_in_respective_responses because there is no guarantee that float values would always be exactly the same.
So do match calculations here require some tolerance-adjustments as well? Wouldn't small mismatch along the line throw all calculations off?

achaikou · 2026-08-26T15:35:56Z

+    raw_key_index = observation_data.loc["key_index"].to_numpy()
+    key_index = _to_matchable_key(raw_key_index)
+    series = pd.Series(obs_values, index=key_index)
+    return series[~series.index.duplicated(keep="first")].dropna()


When can index be duplicated?
Index by definition is supposed to be unique, so this line is suspicious.

I only see this happening because observation data is stacked together from several ensembles (that can in theory belong to different experiments).
But if in ensemble A for index 100 value is 10 and in ensemble B for index 100 value is 10000, it does not seem sensible that responses from B would be matched with observations from A because we kept first value.

One es_mda ensemble selected:

Two ensembles selected:

es_mda suddenly mutated into values from selected ensemble because only first values for index key were kept.

achaikou · 2026-08-26T15:44:02Z

+def _match_obs_to_responses(
+    ensemble_data: pd.DataFrame, obs_by_key_index: pd.Series
+) -> tuple[np.ndarray, np.ndarray]:
+    responses = ensemble_data.copy()


I wonder if we would still need to copy this if we manage to do something with the keys, so that there would be no need to exchange columns with _to_matchable_key

(yes, I struggle to understand why we need _to_matchable_key in the first place 😄 )

achaikou · 2026-08-28T10:50:37Z

+    mock_plot_api_cls = MagicMock(spec=PlotApi)
+    mock_plot_api = MagicMock(spec=PlotApi)
+    mock_plot_api_cls.return_value = mock_plot_api
+
+    storage_version = "0.0"
+    mock_plot_api.api_version = storage_version
+    monkeypatch.setattr(
+        "ert.gui.plotting.plot_window.get_storage_api_version",
+        lambda: storage_version,
+    )
+    monkeypatch.setattr(
+        "ert.gui.plotting.plot_window.PlotApi",
+        mock_plot_api_cls,
+    )


Those lines appear in each test, right?
Also setting of attributes on mock_plot_api happens everytime too.

Is there an option to extract those to a helper like setup_mock_plot_api(responses_key_def=None, ensembles_return_value=None, ...) or is something preventing it from looking and working nicely?

It just seems like half of the test is just an uninteresting mocking setup 😄

achaikou · 2026-08-28T10:57:54Z

+
+
+@pytest.mark.slow
+def test_that_realization_selector_ignores_hidden_ensembles_when_computing_max_size(


What are these hidden ensembles and how can I create one in GUI?
I've never heard of those before! 🐈

Also wonder if this setup deserves it own test or if it can be added to a previous one with hidden ensemble size=20 to assure that no, still just 8 realizations are visible.

As far as I understood things:
Hidden means the ensemble is built from storage, plot_api.py has a line defining an ensembleObject as hidden if the name starts with a . or if the ensemble is undefined (e.g. an experiment/ensemble is created but not yet run).

So I think it makes sense to test that these are not included in the realization selector, but I agree that it might make sense to move it into the previous test.

achaikou · 2026-08-28T11:01:49Z

+    assert plot_window._realization_group.isVisible() is False
+
+
+@pytest.mark.slow


Why is it slow? Is it about switching between tabs or setting up the environment?

Just wonder if this test would become faster if instead of being 4 tests via parametrization we iterate over all tabs in one test itself with 3 tabs being "not expected to show up" and 1 tab being "expected to show up!!".

marked as slow because the tests need to wait until a specific tab is active and therefore working with timeouts (I have seen that with other tests in test_plot_window.py as well, It only enables you to either specifically run slow tests or to skip slow tests, as far as I read up on it.)

But yes this could potentially be moved into one test and done via parameterization

achaikou · 2026-08-28T11:10:55Z

+    )
+
+
+@pytest.mark.slow


Not sure if it would help with speeding it up a bit, but maybe this test could be combined with previous one and test that cross plot is enabled only for seismic keys?
Basically create two keys - seismic and summary - in one test and just check that when seismic key is enabled there is one setup, when summary key is enabled, there is a different setup?

Yeah sounds like a good idea!

achaikou · 2026-08-28T11:13:03Z

+    assert labels == [str(i) for i in range(3)]
+
+
+def test_that_realization_selection_group_is_hidden_on_plot_window_startup(


I wonder if this test couldn't easily be a part of
test_that_realization_selection_group_is_visible_only_when_cross_plot_tab_is_active

It if it only visible when cross_plot tab is active, then name applies it should be hidden at startup 😄

But if you prefer to have these separate, no complains from me! 👍

akustn self-assigned this Aug 24, 2026

ertomatic mentioned this pull request Aug 24, 2026

Update screenshots for Seismic cross plot (equinor/ert#14266) equinor/ert-testdata#92

Open

ertomatic pushed a commit to equinor/ert-testdata that referenced this pull request Aug 25, 2026

Update screenshots for equinor/ert#14266

70bad64

akustn added 4 commits August 25, 2026 13:47

Add seismic cross plot

0689284

Add cross plot tests

cd91717

fixup! Add seismic cross plot

6c0421d

fixup! Add cross plot tests

f7d5f5f

akustn force-pushed the seismic-cross-plot branch from 53aee8f to f7d5f5f Compare August 25, 2026 11:51

ertomatic pushed a commit to equinor/ert-testdata that referenced this pull request Aug 25, 2026

Update screenshots for equinor/ert#14266

d4b58d5

akustn requested a review from achaikou August 25, 2026 12:41

achaikou reviewed Aug 28, 2026

View reviewed changes



		@pytest.mark.slow
		def test_that_realization_selector_ignores_hidden_ensembles_when_computing_max_size(

		assert plot_window._realization_group.isVisible() is False


		@pytest.mark.slow

		assert labels == [str(i) for i in range(3)]


		def test_that_realization_selection_group_is_hidden_on_plot_window_startup(

Uh oh!

Conversation

akustn commented Aug 24, 2026

When applicable

Uh oh!

codecov-commenter commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ertomatic commented Aug 24, 2026

Uh oh!

codspeed-hq Bot commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Uh oh!

achaikou left a comment

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!

achaikou Aug 28, 2026 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Aug 24, 2026 •

edited

Loading

codspeed-hq Bot commented Aug 25, 2026 •

edited

Loading

achaikou Aug 28, 2026 •

edited

Loading