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

Address high CPU usage for some (GUI) simulations #541

Merged
merged 1 commit into from
Nov 13, 2019

Conversation

jondequinor
Copy link
Contributor

@jondequinor jondequinor commented Nov 1, 2019

Issue
Resolves #380

Approach
Splits *UI rendering into three parts: ticker, progress and details.
Each has its own interval at which it renders, which is scaled based on
the number of realizations.

A simulation tracker has been implemented, and it now controls the flow
of information to both the GUI and CLI monitoring through events.

@jondequinor jondequinor force-pushed the address-high-cpu-usage-380 branch 2 times, most recently from 7e3f020 to 6f0748b Compare November 1, 2019 11:05
@jondequinor jondequinor force-pushed the address-high-cpu-usage-380 branch 4 times, most recently from 2c51e53 to 0b598a4 Compare November 4, 2019 12:34
@jondequinor jondequinor changed the title [WIP] Address high CPU usage for some (GUI) simulations Address high CPU usage for some (GUI) simulations Nov 5, 2019
@jondequinor jondequinor force-pushed the address-high-cpu-usage-380 branch 5 times, most recently from fc7d01c to 6aaee2e Compare November 6, 2019 06:48

def _scale(nr_realisations, min_time=5, max_time=15, min_real=1, max_real=500):
nr_realisations = min(max_real, nr_realisations)
nr_realisations = max(min_real, nr_realisations)
Copy link
Contributor

@oysteoh oysteoh Nov 7, 2019

Choose a reason for hiding this comment

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

does not quite get what these two assignments does... ? or - at least the second one always overwrite the first one - guess that should not be the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oyvindeide wanna chime in? =)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, got the explanation which gave sense ... just didn't catch it initially when reading the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Late to the party (and just adding this in case it improves readability), but is the equivalent of np.clip(number, min_real, max_real), or max(min_real, min(number, max_real)), the point is to have a hard upper and lower boundary so that we normalize to 0-1 later.

Copy link
Contributor

@oysteoh oysteoh Nov 11, 2019

Choose a reason for hiding this comment

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

yes, I understand - and it makes sense... just me reading through a bit fast and didn't really got the sense of what's going on. One could probably make this obvious by having

def clamp(nr_realisations, max_real, min_real): 
    clamped = min(max_real, nr_realisations)
    clamped = max(min_real, clamped)
    return clamped

but - guess that is smak & behag

@oysteoh
Copy link
Contributor

oysteoh commented Nov 8, 2019

I'm ready to approve, but I think this PR is quite big and it would have been nice having another pair of eyes running through as well.

@@ -2,7 +2,7 @@
import sys

try:
from PyQt4.QtCore import Qt, QTimer, QSize
from PyQt4.QtCore import Qt, QTimer, QSize, pyqtSignal, pyqtSlot
Copy link
Contributor

Choose a reason for hiding this comment

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

When touching this, could we switch to ErtQt?

Copy link
Contributor

@markusdregi markusdregi left a comment

Choose a reason for hiding this comment

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

This looks really good to me 🥇

Could you make the TODO into an issue as well as another issue on enabling black for the tracker module?

ert_shared/tracker/base.py Show resolved Hide resolved
@jondequinor jondequinor force-pushed the address-high-cpu-usage-380 branch from 6aaee2e to 8c98903 Compare November 13, 2019 11:35
Splits *UI rendering into three parts: ticker, progress and details.
Each has its own interval at which it renders, which is scaled based on
the number of realizations.

A simulation tracker has been implemented, and it now controls the flow
of information to both the GUI and CLI monitoring through events.
@jondequinor jondequinor force-pushed the address-high-cpu-usage-380 branch from 8c98903 to 8ed2b5e Compare November 13, 2019 11:55
@jondequinor jondequinor merged commit a48cc15 into equinor:master Nov 13, 2019
@jondequinor jondequinor deleted the address-high-cpu-usage-380 branch November 13, 2019 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERT client high CPU usage
5 participants