-
Notifications
You must be signed in to change notification settings - Fork 463
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
Add active attribute to components #1038
Conversation
328891d
to
a11c7dc
Compare
problem.lp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups :)
pypsa/components.py
Outdated
list_name: str | ||
attrs: pd.DataFrame | ||
df: pd.DataFrame | ||
pnl: Dict | dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we drop dict
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. But one bigger concern:
We are mixing n.active
, n.active_in_investment_period
and n.get_active_assets
and neither the naming is clear nor is it apparent what the difference is. I would merge them all into one n.active_assets
and handle if build year/lifetime should be taken into account in there.
Even cleaner would be to write the build year and lifetime into the active column. Then nothing would get mixed up. But that is probably not reasonable within the current structure, which makes a clean API important.
Otherwise, thanks for the great cleanup. I am not a big fan of the dataclass though. It starts to implement something without changing all the underlying structural issues and nested initialization. But I guess that's okay since it doesn't touch the API.
pypsa/components.py
Outdated
@property | ||
def network(self) -> Network: | ||
return self._network() # type: ignore | ||
def active_in_investment_period( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe merge that as an optional argument into active_assets
?
Also active_asserts_in_investment_period
instead, but I think a combined method is cleaner
def get_active_assets(n: Network, c: str, investment_period: ArrayLike) -> pd.Series: | ||
def get_active_assets( | ||
n: Network | SubNetwork, c: str, investment_period: ArrayLike | None = None | ||
) -> pd.Series: | ||
""" | ||
Getter function. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs update, specially since we mix active
and the build_year
/ life_time
now and not write back to one
thanks for the comments @lkstrp! As for the active assets function, what is needed quite regularly is the index of active assets. With the current
which I shortcut to
but I agree this is confusing. I will revert that and only define |
For more background: The general issue here is that the activity of an asset has two sources of information a) the global (new) attribute b) the investment year dependent activity which adds on top and is only considered when doing and multi investment optimization. This is dependent on build_year (input), life_time (input) and investment_year (function argument). When thinking about different approaches, my feeling is that the rule that static |
a11c7dc
to
bf83e17
Compare
Co-authored-by: Lukas Trippe <[email protected]>
for more information, see https://pre-commit.ci
…global active attribute
and about the comment about the dataclass, I am happy to remove the dataclass decorator if this is what you meant |
Na, keep it is a it is. It is also so much cleaner than before |
pypsa/components.py
Outdated
if investment_period is None: | ||
return self.df.active |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check can be ignored now
As discussed:
|
looks great, thanks lukas! |
This PR introduces a new component
active
which, as discussed, allow to enable/disable assets from pypsa core functionalities. These areWhen set to inactive, the optimization masks the corresponding asset out. The asset then still appears in the index but does not contain any value. To save memory (in particular for the myopic optimization), we might change this to a harder filter such that inactive assets do not even appear in the variable blocks in the first place. My preference would be to tackle that after #848.
The PR further introduces as
Component
class. Note that this is purely non-breaking and non-intrusive. We had aComponent
object before, which was anamedtuple
. With the new class we gain more control when iterating over components. It might as well pave the way to a more formalComponent
behaviour as discussed on #936.(Do not merge yet, I would like to see some more tests on the optimization and power flow.)
Checklist
doc
.environment.yaml
,environment_docs.yaml
andsetup.py
(if applicable).doc/release_notes.rst
of the upcoming release is included.