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

Account for remaining values in multi-period models #982

Merged

Conversation

jokochems
Copy link
Member

  • Subtract remaining values for investments expressing their value at the end of the optimization horizon.
  • Limit fixed costs to actual optimization horizon.

@jokochems
Copy link
Member Author

I'll update the tests shortly. Feel free to already have a glimspe into the implementation. In case I haven't been mistaken, it hasn't been too hard to implement.

@jokochems
Copy link
Member Author

After checking back with @nailend, I'll revise the implementation and guess that it gets leaner through the changes we discussed bilaterally.

@jokochems
Copy link
Member Author

@nailend Please have a look when time allows:

  • As we discussed, I limited the (per year) annuities and (per year) fixed costs to the optimization horizon.
  • This way, there is no (urgent) need to account for remaining values.

@p-snft
I see two ways to account for remaining values:

  1. Assume that investment expenses are "locked in".
  2. Account for changes in investment expenses over time - what you suggested.

In terms of implementation, I also see two ways:

  • A. Limit to payments that happen within the optimization horizon.
  • B. Consider all payments over the lifetime and subtract the outside of optimization horizon payments again.

Here is how I proceeded:

  • Starting off, I wanted to go for approach 2.B. and tried to implement that.
  • In the course of the discussion with @nailend, we found that approach A. would lead to cleaner code and processing (e.g. not having to account for outside horizon fixed costs as it was before).
  • Thus, I went for approach 1.A. in the first place. The main reason is that it was easier to implement, compared to 2.A. Alongside, I also fixed some bugs of the previous implementation.

In principle, we could also go for approach 1.A., but there are some design questions:

  • We would need to compare at the end of the optimization horizon and add/subtract the difference in value. I'd have a rough idea how to do that.
  • Additionally, we would require the information of the investment expenses at the end of the last period, i.e. the beginning of the next upcoming one that we do not simulate. While in principle, this information could be passed, it might also be somewhat counter-intuitive and potentially error-prone that you have to provide p + 1 instead of p periods.

What do you think, @p-snft?

@jokochems
Copy link
Member Author

@p-snft Adding to what I stated above, I gave it some thought:

  • What I have included in this pull request and what was the main motivation is actually rather a bug fix to prevent the bias towards earlier investments in the optimization horizon.
  • Considering the cost change over time when evaluating an actual remaining value, to me is rather a feature. Thus, I suggest to leave it out here and open a dedicated pull request for that. Does this make sense to you?

@p-snft
Copy link
Member

p-snft commented Oct 6, 2023

@p-snft Adding to what I stated above, I gave it some thought:

What I have included in this pull request and what was the main motivation is actually rather a bug fix to prevent the bias towards earlier investments in the optimization horizon.

Considering the cost change over time when evaluating an actual remaining value, to me is rather a feature. Thus, I suggest to leave it out here and open a dedicated pull request for that. Does this make sense to you?

I agree. Actually, my suggestion was rather meant as a quick fix: Instead of calculating the remaining value at the end of the time horizon, it would just have been left to the user to give it. Maybe, it can be considered the sloppy version of 2A.

@jokochems jokochems marked this pull request as ready for review October 6, 2023 16:32
@jokochems jokochems requested a review from nailend October 6, 2023 16:32
@jokochems jokochems marked this pull request as ready for review October 13, 2023 11:23
@jokochems jokochems requested a review from nailend October 13, 2023 11:53
@jokochems
Copy link
Member Author

jokochems commented Oct 13, 2023

@nailend could you please re-review? @p-snft Could you add your review as well or accept in case @nailend finds nothing more that needs to be changed?

FYI: The failing docs test seems to be due to a failing docs test for a zenodo link. I assume it to be a temporary server problem for zenodo. [EDIT] They just migrated and announced that they are still working on slowness issues. I guess, the server timeout limit was just too harsh.

Thank you!

@jokochems
Copy link
Member Author

@nailend and @p-snft: I added additional functionality, namely allowing to re-evaluate assets at the end of the optimization horizon as an (optional) feature, as @p-snft suggested in the issue. Please have a look at this as well. Thank you!

@p-snft
Copy link
Member

p-snft commented Oct 13, 2023

I am just wondering if it is possible to move the economic calculations to https://github.com/oemof/oemof-tools/blob/dev/src/oemof/tools/economics.py. (That might be another PR, though.)

@jokochems
Copy link
Member Author

I am just wondering if it is possible to move the economic calculations to https://github.com/oemof/oemof-tools/blob/dev/src/oemof/tools/economics.py. (That might be another PR, though.)

Sounds like a good idea, indeed for a future PR.

Copy link
Contributor

@nailend nailend left a comment

Choose a reason for hiding this comment

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

I think it's great! Thanks a lot for all the changes and improvements, this is of great help to us!

src/oemof/solph/flows/_investment_flow_block.py Outdated Show resolved Hide resolved
Copy link
Member

@p-snft p-snft left a comment

Choose a reason for hiding this comment

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

I checked for code style and if it breaks "stable" functionality. I see no real issues with that regard. There is no reason I see against the merge, I only have small remarks.

Comment on lines 87 to +90
fixed_costs : numeric (iterable or scalar), :math:`c_{fixed}`
The fixed costs associated with a flow.
Note: These are only applicable for a multi-period model.
Note: These are only applicable for a multi-period model
and given on a yearly basis.
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering whether we should re-introduce this for single-period models.

Copy link
Member Author

Choose a reason for hiding this comment

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

That has been a discussion in a very old issue #556. I generally like the idea as well, but did not do so, because my intention was to add functionality, not to take major design choices for the existing framework.

src/oemof/solph/flows/_investment_flow_block.py Outdated Show resolved Hide resolved
src/oemof/solph/flows/_investment_flow_block.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Oct 20, 2023

Hello @jokochems! 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 2023-10-20 08:32:53 UTC

@jokochems
Copy link
Member Author

Thanks a lot for your reviews, suggestions and approvals, @nailend and @p-snft. I will merge this back.

@jokochems jokochems merged commit 119e9cb into dev Oct 20, 2023
12 checks passed
@p-snft p-snft deleted the feature/account-for-remaining-values-of-multi-period-investments branch October 20, 2023 18:28
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.

Multi-period investments are biased since the remaining value is not accounted for
4 participants