-
Notifications
You must be signed in to change notification settings - Fork 20
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 simple method for multi year investment on assets #793
Add simple method for multi year investment on assets #793
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #793 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 18 18
Lines 904 915 +11
=========================================
+ Hits 904 915 +11 ☔ View full report in Codecov by Sentry. |
@abelsiqueira depending on the answer from @gnawin here #462 (comment) the code might simplify a bit more depending if we don't allow missing values on the investment method. But, for now, I would appreciate your feedback. Also, these changes might help to have a better overview for the ones in #784 |
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.
Thanks for the PR. I noticed a place where we can join the sum
s. It shouldn't be a performance bottleneck, so you can choose whatever makes the code more readable.
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.
Hi @datejada thanks for the PR, I left a few comments. We can discuss in person today if you want.
@gnawin, then let's review it together in person because the code reflects the equations that are in the pdf 🤷 |
@gnawin, my takeaway is that we might need an extra parameter to cover the cases you describe more easily. Otherwise, I don't see a combination that allows existing capacity to be decommissioned without being an investable asset (with the current parameters' definitions). |
…bjective functions" This reverts commit a587303.
542953e
to
7f7f750
Compare
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.
Hi @datejada thanks a lot for the PR!
As discussed, let's for now leave out the upper bounds for the decommission variables and do that in a separate PR.
The naming of things can be improved but let's do it later after my PR.
And regarding your comments on the initial capacity/units, let's continue the discussion but also do it separately.
Co-authored-by: Ni Wang <[email protected]>
Co-authored-by: Ni Wang <[email protected]>
Co-authored-by: Ni Wang <[email protected]>
Co-authored-by: Ni Wang <[email protected]>
Co-authored-by: Ni Wang <[email protected]>
Pull request details
Describe the changes made in this pull request
Changes to consider the simple method in the investment of assets.
Other related issues are:
They are done in separate issues to keep the changes as atomic as possible.
List of related issues or pull requests
Closes #786
Collaboration confirmation
As a contributor I confirm