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.

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

Replacing progress view with progress widget #8152

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

andreas-el
Copy link
Contributor

@andreas-el andreas-el commented Jun 14, 2024

Replace LegendView and ProgressView with a simple ProgressWidget.

Visually the end result should be the same, but the complexity underneath should be greatly simplified.
Now base_run_model will calculate the necessary numbers once, and the widget will use these for drawing the components.

If anything, the new code seems crisper in the text, and also does not shave off the very left side of the green finished label marker.

New
Screenshot 2024-06-14 at 11 27 58

Old
Screenshot 2024-06-14 at 11 30 51

  • 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 tests pass locally (after every commit!)

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

@andreas-el andreas-el force-pushed the refactor_run_dialog_visuals branch 2 times, most recently from 76d53f6 to 3678c91 Compare June 14, 2024 07:04
@andreas-el andreas-el added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Jun 14, 2024
@andreas-el andreas-el self-assigned this Jun 14, 2024
@andreas-el andreas-el force-pushed the refactor_run_dialog_visuals branch 2 times, most recently from b675d75 to 7660539 Compare June 14, 2024 08:29
Comment on lines 53 to 62
spread = realization_count
gen_list = []

for _ in range(len(REAL_STATE_TO_COLOR)):
r = randint(0, spread)
gen_list.append(r)
spread -= r

for i, state in enumerate(REAL_STATE_TO_COLOR.keys()):
status[state] = gen_list[i]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could use hypothesis here to generate input.
We just need to retain some sensical sum for the amount of realizations. Once this shows unrealistically high numbers, the visuals will not add up perfectly. For instance if the number of reals is way more than number of pixels available.

Copy link
Contributor

Choose a reason for hiding this comment

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

Came up with this

from hypothesis import given, settings, HealthCheck
import pytest
import hypothesis.strategies as st

from qtpy.QtWidgets import QLabel

from ert.ensemble_evaluator.state import REAL_STATE_TO_COLOR
from ert.gui.simulation.view import ProgressWidget
from tests.unit_tests.gui.conftest import get_child


def test_progress_step_changing(qtbot):
    progress_widget = ProgressWidget()
    qtbot.addWidget(progress_widget)

    realization_count = 1

    for i in range(len(REAL_STATE_TO_COLOR.keys())):
        status = {}

        # generate new list with one flag set
        for u, state in enumerate(REAL_STATE_TO_COLOR.keys()):
            status[state] = 1 if i == u else 0

        progress_widget.update_progress(status, realization_count)

        for state in REAL_STATE_TO_COLOR:
            label_marker = get_child(
                progress_widget,
                QLabel,
                name=f"progress_label_text_{state}",
            )

            assert label_marker
            count = status[state]
            assert f" {state} ({count}/{realization_count})" in label_marker.text()


@given(
    realization_count=st.integers(min_value=3, max_value=100),
    status=st.lists(st.sampled_from(list(REAL_STATE_TO_COLOR.keys()))),
)
@settings(suppress_health_check=[HealthCheck.function_scoped_fixture])
def test_progress_state_width_correct(qtbot, status, realization_count):
    progress_widget = ProgressWidget()
    qtbot.addWidget(progress_widget)
    status = {"Unknown": realization_count}
    progress_widget.update_progress(status, realization_count)

    progress_marker = get_child(
        progress_widget,
        QLabel,
        name="progress_Unknown",
    )

    assert progress_marker
    base_width = progress_marker.width() / realization_count
    progress_widget.update_progress(status, realization_count)

    for state in REAL_STATE_TO_COLOR:
        progress_marker = get_child(
            progress_widget,
            QLabel,
            name=f"progress_{state}",
        )

        assert progress_marker
        count = status.get(state, 0)
        assert progress_marker.width() == pytest.approx(base_width * count)

Copy link
Contributor

Choose a reason for hiding this comment

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

After talking we found this:

from hypothesis import given, settings, HealthCheck
from typing import Dict
import pytest
import hypothesis.strategies as st

from qtpy.QtWidgets import QLabel

from ert.ensemble_evaluator.state import REAL_STATE_TO_COLOR
from ert.gui.simulation.view import ProgressWidget
from tests.unit_tests.gui.conftest import get_child


@given(realization_count=st.integers(min_value=3, max_value=100))
@settings(suppress_health_check=[HealthCheck.function_scoped_fixture])
def test_progress_step_changing(qtbot, realization_count):
    progress_widget = ProgressWidget()
    qtbot.addWidget(progress_widget)

    for i in range(len(REAL_STATE_TO_COLOR.keys())):
        status = {}

        # generate new list with one flag set
        for u, state in enumerate(REAL_STATE_TO_COLOR.keys()):
            status[state] = 1 if i == u else 0

        progress_widget.update_progress(status, realization_count)

        for state in REAL_STATE_TO_COLOR:
            label_marker = get_child(
                progress_widget,
                QLabel,
                name=f"progress_label_text_{state}",
            )

            assert label_marker
            count = status[state]
            assert f" {state} ({count}/{realization_count})" in label_marker.text()


@given(
    status=st.dictionaries(
        st.sampled_from(list(REAL_STATE_TO_COLOR.keys())),
        st.integers(min_value=1, max_value=10),
        min_size=1,
    )
)
@settings(suppress_health_check=[HealthCheck.function_scoped_fixture])
def test_progress_state_width_correct(qtbot, status: Dict[str, int]):
    realization_count = sum(status.values())
    progress_widget = ProgressWidget()
    qtbot.addWidget(progress_widget)
    status = {"Unknown": realization_count}
    progress_widget.update_progress(status, realization_count)

    progress_marker = get_child(
        progress_widget,
        QLabel,
        name="progress_Unknown",
    )

    assert progress_marker
    base_width = progress_marker.width() / realization_count
    progress_widget.update_progress(status, realization_count)

    for state in REAL_STATE_TO_COLOR:
        progress_marker = get_child(
            progress_widget,
            QLabel,
            name=f"progress_{state}",
        )

        assert progress_marker
        count = status.get(state, 0)
        assert progress_marker.width() == pytest.approx(base_width * count)

@andreas-el andreas-el force-pushed the refactor_run_dialog_visuals branch 2 times, most recently from 93b2d28 to 7ac6cdd Compare June 14, 2024 10:02
@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.53%. Comparing base (77fa511) to head (2ec8244).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8152      +/-   ##
==========================================
+ Coverage   76.93%   86.53%   +9.60%     
==========================================
  Files         383      382       -1     
  Lines       23813    23749      -64     
  Branches      625      628       +3     
==========================================
+ Hits        18320    20552    +2232     
+ Misses       5412     3124    -2288     
+ Partials       81       73       -8     

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

@andreas-el andreas-el force-pushed the refactor_run_dialog_visuals branch from 7ac6cdd to 2ec8244 Compare June 17, 2024 11:20
Copy link
Contributor

@eivindjahren eivindjahren left a comment

Choose a reason for hiding this comment

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

👍

@andreas-el andreas-el merged commit 14dcf00 into equinor:main Jun 18, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:maintenance Automatically categorise as maintenance change in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants