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

Fix y intercept of OffsetConverter #1012

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Conversation

p-snft
Copy link
Member

@p-snft p-snft commented Nov 15, 2023

Fixes the OffsetConverter to work as documented.

Closes #1010

@p-snft p-snft requested a review from lensum November 15, 2023 15:19
@p-snft p-snft self-assigned this Nov 15, 2023
Copy link
Contributor

@lensum lensum left a comment

Choose a reason for hiding this comment

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

Thank you for the quick fix! I haven't looked into the .lp files in detail, but the changes you've made make sense to me.

@p-snft p-snft merged commit 72bfb3b into dev Nov 15, 2023
12 checks passed
@Bachibouzouk
Copy link
Contributor

Hello, I actually disagree that this produces the right behavior, I had problems reproducing the graphs of #895 and I stumbled upon this PR.

In the documentation is it stated that P_in(p,t) = P_out(p,t) * C_1(t) + C_0(t) * P_max(p) * Y(t) and it is explicitly stated that P_max(p) * Y(t) is equivalent to status_nominal[n, o, p, t]

I noticed that the beginning of the _relation_rule loop was wrong (

)

It reads

expr = 0
expr += -m.flow[n, o, p, t]  # this is P_out(p,t) and it is not the one multiplied by C_1(t) !
expr += m.flow[i, n, p, t] * n.coefficients[1][t]

instead of

expr = 0
expr += -m.flow[i, n, p, t]
expr += m.flow[n, o, p, t] * n.coefficients[1][t]

I did locally reversed the changes of this PR (replacing status by nominal_status) and implemented the switch of P_in and P_out and I could reproduce the figures of #895 successfully, which was not the case with the code of this PR.

I need to look at it again with a fresh brain, after considering the points raised in #1010 as well

p-snft added a commit that referenced this pull request Jan 12, 2024
…tercept"

This reverts commit 72bfb3b, reversing
changes made to 7c7ba3b.
@p-snft p-snft deleted the fix/offset_converter_y-intercept branch May 16, 2024 12:17
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.

Misleading/missing documentation of OffsetConverter
3 participants