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.

Sign up for GitHub

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

Jump to bottom

Remove duplicated discounting #1028

Merged
merged 8 commits into from Dec 5, 2023
Merged

Remove duplicated discounting #1028

merged 8 commits into from Dec 5, 2023

Conversation

nailend
Copy link
Contributor

@nailend nailend commented Nov 29, 2023 •
edited

close #1019

Remove duplicated discounting for fixed costs.

The discounting is already taken care of as the discount starts with the temporal distance of optimization start and investment period.

@nailend
Remove duplicated discounting
69b66e9 
The discounting is already taken care of by the
list comprehension above. As it discount starting
with the temporal distance of optimization start
and investment period.
@nailend
Copy link
Contributor Author

nailend commented Nov 29, 2023 •
edited

No changes in any tests needed....

Shouldn't this have an effect on the objective and therefore change the LP file for any Investment test with fixed costs?

@jokochems
Copy link
Member

No changes in any tests needed....

Shouldn't this have an effect on the objective and therefore change the LP file for any Investment test with fixed costs?

Yes, it should. I was a bit shocked that there is no effect at first, but a brief glimpse into the code shed some light on it:

  • This is not the only code occurence. It needs to be fixed for storages and SinkDSM as well.
  • For the multi-period constraint tests, there are simply no tests, yet, that test fixed costs integration for investment flows.
  • They are used in the "full-featured toy model" test_multi_period_investment_model.py, but the test does only check for the optimization outcome. Supposingly, the bug was "not that bad" that it led to a deviating optimization outcome in terms of investments taken.

Thus:

  • I corrected it for the storage and SinkDSM as well. - Please check: It was a real quick'n'dirty search for code occurences.
  • Would you mind adjusting the tests accordingly?
  • I'm happy to add my review then, but as I mentioned in another issue, unfortunately, currently my time ressources are quite limited ... sorry about that and thanks a ton for detecting and fixing that issue!

@jokochems jokochems marked this pull request as draft November 29, 2023 15:22
@nailend
Remove comma
5f0c400
@nailend
Adjust LP-files to fixed fixed costs discounting
e8e4956
@nailend
Add fixed costs to multi-period converter tests
bbc34ac 
Fixed costs were not included in the converter test and
thus changes in fixed costs discounting not tested before.
@nailend
Copy link
Contributor Author

nailend commented Dec 5, 2023 •
edited

I updated the LP files and checked them roughly. I also added fixed costs to the multi-period converter tests, this way the changes for the discounting are also checked with "standard" components.

@nailend nailend marked this pull request as ready for review December 5, 2023 16:28
@nailend
Fix LP file due to pyomo6.7 changes
672155c 
Former zero floats are now zero integers in the LP file.
@nailend
Copy link
Contributor Author

nailend commented Dec 5, 2023 •
edited

pytest is now failing due to changes in pyomo which sometimes prints zero integers instead of floats in the LP-files. Not sure if this should be done separately as it's unrelated changes... I fixed it now in this PR as it was only one LP file @p-snft, what do you think?

@nailend nailend self-assigned this Dec 5, 2023
@nailend nailend added the bug label Dec 5, 2023
@p-snft
Copy link
Member

p-snft commented Dec 5, 2023

I fixed the same thing in another branch. XD

jokochems
jokochems approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@jokochems jokochems left a comment

Choose a reason for hiding this comment

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

Thanks @nailend. I can approve it. Once the fix for the test has been included from that other feature branch (@p-snft Could you please add that or post a link to the PR you are referring to? I vaguely remember your efforts to align with the new pyomo lp writer ...), in my view it should be merged.

@p-snft
Copy link
Member

p-snft commented Dec 5, 2023

Well, no, the other thing was bigger and has been merged a long time ago. I just replaces 0.0 by 0 in #1021 (commit 22abd38).

@nailend nailend merged commit 2e9ea5e into dev Dec 5, 2023
14 checks passed
@nailend nailend deleted the fix/multi-period-investment-fixed-costs branch December 5, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jokochems jokochems jokochems approved these changes

Assignees

@nailend nailend

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Discounting duplication for fixed costs
3 participants
@nailend @jokochems @p-snft