Skip to content

Extend get_plotting_data method of Drum class. #521

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

Merged
merged 5 commits into from
Jun 15, 2024
Merged

Conversation

jfreissmann
Copy link
Collaborator

General

The get_plotting_data method of the Drum class only returns one process line connecting the saturated liquid 'out1' with the saturated vapor 'out2' so far. This can work sometimes, because the input saturated liquid-vapor mixture falls somewhere between these points, but the desuperheating of the gas 'in2' is lost and can not be shown. Unfortunately, the Drum component does not lend itself to a clear fix that is in line with other components and the general discription of the method. That is, because changing the returned dict to contain one line from 'in1' to 'out1' and one from 'in2' to 'out2' loses the depiction of the isolation of the saturated vapor from the mixture. Therefore, I propose this solution connecting every inlet to every output and letting the user decide which lines they want to plot.

What do you think about this? Either way, in my opinion the old version can not stay, as you can not show the desuperheating. Additionally, the method desciption did not match the return value in the case of the Drum.

@pep8speaks
Copy link

pep8speaks commented Jun 12, 2024

Hello @jfreissmann! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 431:80: E501 line too long (80 > 79 characters)

Comment last updated at 2024-06-15 06:12:30 UTC

@fwitte
Copy link
Member

fwitte commented Jun 13, 2024

Hi @jfreissmann,

thanks a lot for your contribution. I will look into the changes later and let you know how we proceed:).

Best

Copy link
Member

@fwitte fwitte left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! Can you change it, so the key 1 remains the old version (outlet 1 to outlet 2). We can add a warning message, that this behavior will change in the future and change it with the next major release.

@fwitte fwitte merged commit 95196fe into oemof:dev Jun 15, 2024
6 of 7 checks passed
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.

3 participants