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

chore: include invalid data in pump model plot #431

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

frodehk
Copy link
Contributor

@frodehk frodehk commented Apr 11, 2024

ECALC-799

Why is this pull request needed?

The lower-level results plots currently differ between ‘compressors’ and ‘pumps’. The heading on both power plots state that invalid points are included, however, that is only true for compressors. Pump model plots should also include invalid points.

Issues related to this change:

https://equinor-ecalc.atlassian.net/browse/ECALC-799?atlOrigin=eyJpIjoiYzY1YzExNWJkZTcwNDk2Nzk2MGJmODJmZGVhYTk5OTUiLCJwIjoiaiJ9

@frodehk frodehk self-assigned this Apr 11, 2024
@frodehk frodehk requested a review from a team as a code owner April 11, 2024 11:33
PumpFlag.INVALID_REQUIRED_HEAD_ABOVE_ACTUAL_HEAD.value,
pump_flags,
)

Copy link
Contributor

Choose a reason for hiding this comment

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

PumpModelResults inherits is_valid from EnergyFunctionResult and is basically is_valid = False when values are nan. I guess you need a new is_valid property on PumpModelResult using the pump_flags you have defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, that is probably what we need.

@frodehk frodehk force-pushed the ECALC-799-include-invalid-data-pump-model-plot branch from e3880d0 to 078ffb4 Compare April 24, 2024 07:49
@frodehk frodehk marked this pull request as draft October 2, 2024 09:03
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.

2 participants