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

Features/refine investments #753

Draft
wants to merge 5 commits into
base: dev
Choose a base branch
from
Draft

Conversation

p-snft
Copy link
Member

@p-snft p-snft commented Mar 13, 2021

This PR suggests a new API for investments:

  • Investment has a new keyword argument other_needs. It replaces custom kwargs that served the same purpose.
  • The constraints investment_limit and additional_investment_flow_limit are unified. If a keyword is named, the corresponding entry in other_needs is considered, else the summed ep_costs are limited.

@pep8speaks
Copy link

pep8speaks commented Mar 13, 2021

Hello @p-snft! 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 2021-03-15 15:47:39 UTC

@p-snft
Copy link
Member Author

p-snft commented Mar 14, 2021

I am still struggling with the naming of the „other needs“. There are a couple of options:

  • Demands (should probably be reserved for energy covered by flows).
  • Needs
  • Requirements
  • Prerequisites

@jokochems
Copy link
Member

I am still struggling with the naming of the „other needs“. There are a couple of options:

* Demands (should probably be reserved for energy covered by flows).

* Needs

* Requirements

* Prerequisites

If I understood it correctly, you use other_needs to impose other limiting factors to investments than the total investment costs, right?
In that case, I would rather not associate it with requirements or needs, but rather suggest

  • limiting_factors
  • other_aspects
  • boundary_args

or something like that.

@p-snft
Copy link
Member Author

p-snft commented Mar 15, 2021

If I understood it correctly, you use other_needs to impose other limiting factors to investments than the total investment costs, right?

It will be used for limiting factors that are shared between Storages and Flows. However, there is no strict requirement that a limit is set. Thus, it can be used to calculate things for information only. (I.e. to have the space demand at hand, saved in a variable.)

@uvchik
Copy link
Member

uvchik commented Mar 15, 2021

I cannot see the changes made by this PR due to hundreds of changed lines that do not belong to this PR.

However, I like custom_kwargs as it is a placeholder for custom keyword arguments that become attributes and can be used in custom constraints.

@p-snft
Copy link
Member Author

p-snft commented Mar 15, 2021

I cannot see the changes made by this PR due to hundreds of changed lines that do not belong to this PR.

I based this one on #752. (We agreed on the general idea, so it will probably avoid double work.) I hope to have that one finished soon, so all changes specific to this branch can be seen.

However, I like custom_kwargs as it is a placeholder for custom keyword arguments that become attributes and can be used in custom constraints.

Generally, I do agree. However, custom_kwargs would be an argument to the solph non-Block-class. But as far as I see it, for investments to storages there has to be an attribute to GenericInvestStorageBlock. So, to allow identifying keywords that need to be handed to the Block, I suggest to have this extra argument. (Also: investment limits are part of mainline solph, so it's not really a "custom" argument.)

PS: I think, #752 is ready.

@joroeder
Copy link
Member

With the API of other_needs a nested structure is introduced. Maybe, I did not follow all of the discussion, but the purpose with that is to implement explicitly named arguments (#727), right?

@p-snft
Copy link
Member Author

p-snft commented Mar 19, 2021

With the API of other_needs a nested structure is introduced. Maybe, I did not follow all of the discussion, but the purpose with that is to implement explicitly named arguments (#727), right?

There are two reasons:

  1. Arguments providing "core" functionality should have explicit names. (Reject 'variable_cost' on Flow objects #727)
  2. We need a way to signify which arguments need to generate a variable for the optimiser.

Base automatically changed from v0.5 to dev June 24, 2022 10:01
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.

5 participants