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

OffsetTransformer with nonconvex and investment options #861

Closed
sasa821 opened this issue Sep 16, 2022 · 5 comments
Closed

OffsetTransformer with nonconvex and investment options #861

sasa821 opened this issue Sep 16, 2022 · 5 comments
Assignees

Comments

@sasa821
Copy link
Contributor

sasa821 commented Sep 16, 2022

Currently, the OffsetTransformer can be used ONLY when its nominal_value is already known, which means, it is not possible to find the optimal capacity of a transformer with a given efficiency curve (instead of a single efficiency).

Nevertheless, after #856 is finalized, it will be possible to have this functionality. But the OffsetTransformer needs to be adjusted because, in the current version, the nonconvex option MUST be defined for the input flow. This will cause problems if the optimal capacity of the component is unknown. In this situation, the nonconvex option and the investment option must be defined for the output flow. The reason is that the ep_cost in the investment option should apply to the outflow of assets, and it is impossible to map this value for the input flow. Moreover, we cannot define investment in the outflow and nonconvex in the inflow because, in this case, since the nominal_value would be unknown, both these options must be used in the same flow.

I am making some changes in the OffsetTransformer and will soon open a pull request about the proposed changes, but it would be nice to discuss it here, if there is disagreement on this issue.

@joroeder
Copy link
Member

There was a reason putting the offset on the inflow, but I don't remember exactly. It may be that this matters only in the case, if min=0, than there could be outflow at zero inflow, or something like that ..

Nonetheless, there should be nothing against opening a PR. It would be really nice in combination with an investment. It makes for sure sense, if you take the new structure of the flows as basis. If it was really just the case I described above, you could then also issue a warning. Or you make it more general somehow, that you can set the offset arbitrarily to the in- or outflow, that would be cool anyway.

@sasa821
Copy link
Contributor Author

sasa821 commented Oct 7, 2022

@p-snft I would like to finalize the OffsetTransformer according to the new flow structure. Is it OK if I open a new PR in #856, or is it better to wait until #856 is merged into the dev?

@p-snft
Copy link
Member

p-snft commented Oct 10, 2022

@sasa821: You can open a PR against /feature/restructure_flow. I did the same with #836. (If you think #856 is ready to be merged, state it over there and I will do so. I fixed the example you asked about and now I'm just waiting for a positive review.)

@sasa821
Copy link
Contributor Author

sasa821 commented Oct 13, 2022

There was a reason putting the offset on the inflow, but I don't remember exactly. It may be that this matters only in the case, if min=0, than there could be outflow at zero inflow, or something like that ..

Nonetheless, there should be nothing against opening a PR. It would be really nice in combination with an investment. It makes for sure sense, if you take the new structure of the flows as basis. If it was really just the case I described above, you could then also issue a warning. Or you make it more general somehow, that you can set the offset arbitrarily to the in- or outflow, that would be cool anyway.

I believe it would make more sense to define the NonConvex and Investment options, both ONLY on the output flow. Otherwise, the implementation of the required adjustments in the OffsetTransformer would become unnecessarily complicated. It would also be confusing for the users to sometimes use the input flow and sometimes use the output flow as the basis for modeling the efficiency curve. On the other hand, I still do not understand why the outflow should be non-zero if the inflow is zero.

@lensum
Copy link
Contributor

lensum commented Nov 14, 2023

Seeing that the PR went through and that the last interaction was a year ago, I take the liberty to close this issue. Stumbled upon it while researching for #1010

@lensum lensum closed this as completed Nov 14, 2023
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

No branches or pull requests

4 participants