-
Notifications
You must be signed in to change notification settings - Fork 330
enh(add_existing_baseyear): Pull out valid_grouping_years algorithm from heat #1570
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
base: master
Are you sure you want to change the base?
enh(add_existing_baseyear): Pull out valid_grouping_years algorithm from heat #1570
Conversation
…rom heat Apply also to power.
# Installation is assumed to be linear for the past | ||
ratios = _years / _years.sum() | ||
# get number of years of each interval | ||
_years = pd.Index([grouping_years[0] - baseyear + default_lifetime]).append( |
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.
I am a bit confused about this expression here, this can be a negative number or? If grouping_year[0] == 1990 and baseyear == 2020 and lifetime == 20 for example.
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.
I must admit, i did not think about this. It is a literal port of Amos':
# Fill NA from .diff() with value for the first interval
_years[0] = valid_grouping_years[0] - baseyear + default_lifetime
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.
Ok, sorry, Amos, i was wrongly blaming you. The origin story of that expression is complicated.
@lisazeyen came up with:
https://github.com/PyPSA/pypsa-eur/blame/a72388b989d1d667ec7e44d66f6c3b494b46d000/scripts/add_existing_baseyear.py#L466-L467
_years = (valid_grouping_years.diff().shift(-1)
.fillna(baseyear-valid_grouping_years.iloc[-1]))
ie. (baseyear - last of grouping_years) which sounds quite sensible (for an interval, unless the last grouping year is the same as the baseyear).
and then @lindnemi changed it to:
# Fill NA from .diff() with value for the first interval
_years[0] = valid_grouping_years[0] - baseyear + default_lifetime
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.
It sounds like there was some back and forth, i'll not try to understand this fully now, maybe its best to have a short discussion.
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.
Sorry for the late reply @coroa! I think I only made some small refactoring changes here. I'll be OOO for a week but happy to discuss this further then, if helpful.
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.
I haven't really worked with this, so I'm a little out of my comfort zone. I'd just suggest a function docstring for valid_grouping_years
. Otherwise happy if @FabianHofmann is happy.
@lindnemi if it makes sense, we can also have a short call next week to try to see what to do with it, and what would be the ideal aspiration. |
Sorry for the late reply, I totally missed the mentions. If you need some urgent feedback better ping me on discord. Great to see you want to improve the grouping years, here is a bit of context on how the grouping_years work
|
For a proper refactor I would suggest to unify |
Hi @lindnemi , funnily i am now also coming back with only quite a delay to this PR. Thanks for your special case description, ie point 2, i was not aware of this tricky difference. What do you think are the benefits of splitting the extendable capacity from the fixed capacity (with a historical suffix) in comparison to the p_nom/p_nom_min combination? |
And thanks for point 3. That is indeed a mistake. |
Only benefit is that the distinction between historical assets and expanded assets would be clearer, but on second thought it's clear enough already, so let's just leave it as is. The harmonization between electricity and heating sector would have more benefits. |
Changes proposed in this Pull Request
Minimal refactoring to re-use the grouping_years validation also for power. And fix a wrong indent.
Checklist
envs/environment.yaml
.config/config.default.yaml
.doc/configtables/*.csv
.doc/data_sources.rst
.doc/release_notes.rst
is added.