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

Multiple Outputs for OffsetConverter #1054

Merged
merged 9 commits into from
May 16, 2024

Conversation

lensum
Copy link
Contributor

@lensum lensum commented Feb 16, 2024

This PR enables the OffsetConverter to output two (or more) flows with part-load depending efficiencies.

  1. outputs are allowed to contain multiple busses.
  2. coefficients expects a dictionary, where the keys are the output busses and the values are the tuples containing the necessary parameters. I took the liberty to implement a deprecation warning for the old behaviour (passing a tuple without specifying the output bus it corresponds to). This way, the OffsetConverter is more in line with the regular Converter and there is less room for confusion. But this is totally up for debate!
  3. The issues discussed in Misleading/missing documentation of OffsetConverter #1010 are fixed (misleading docu, usage guide and erroneous example).

@pep8speaks
Copy link

pep8speaks commented Feb 16, 2024

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-05-16 19:08:40 UTC

@lensum
Copy link
Contributor Author

lensum commented Feb 18, 2024

Can't figure out what problem tox is having, would be glad for a hint in case someone knows!

@p-snft p-snft linked an issue Feb 20, 2024 that may be closed by this pull request
@p-snft
Copy link
Member

p-snft commented May 16, 2024

I'd suggest to close this and have an example that models the same thing using multiple components instead. @fwitte asked about an electrolyser and waste heat, but the example could also be a CHP with increasing electrical and reducing thermal efficiency towards full load.

@fwitte
Copy link
Member

fwitte commented May 16, 2024

I also quickly looked into the OffsetConverter, and I would now actually suggest implementing this feature (I already did locally before I found this PR)... It is quite straight forward, and it could be done using only a single binary variable.

@p-snft
Copy link
Member

p-snft commented May 16, 2024

I see. Then, I accept this (also as a fix to #1053).

@p-snft p-snft marked this pull request as ready for review May 16, 2024 19:13
@p-snft p-snft merged commit e5fb98c into oemof:dev May 16, 2024
12 checks passed
@fwitte
Copy link
Member

fwitte commented May 16, 2024

I will make another PR, I would like to change the naming conventions and styles of the equations (to have the exact same structure as in the Converter, i.e. using conversion_factors, which should be supplied by a dictionary using the keywords offset and slope or something like that). We can discuss there then :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow multiple outputs at OffsetConverter
4 participants