-
Notifications
You must be signed in to change notification settings - Fork 19
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
Refactor variables to use TulipaVariable #925
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #925 +/- ##
==========================================
+ Coverage 97.85% 98.76% +0.90%
==========================================
Files 29 30 +1
Lines 1214 1210 -4
==========================================
+ Hits 1188 1195 +7
+ Misses 26 15 -11 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Benchmark Results
Benchmark PlotsA plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. |
@datejada @gnawin I translated most investment variables into SQL, but I can't do the same for The main issue is that the variable apparently is not a subset of the rows of assets_data, so I don't have a good intuition on how to start. |
ce6a64c
to
d00dcd1
Compare
# NOTE: To add another column from assets_data, it has to be added on the two inner SELECT | ||
# statements (not the year_data one) | ||
variables[:assets_decommission_compact_method] = TulipaVariable( | ||
DuckDB.query( |
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 @abelsiqueira, @gnawin and I reviewed this query and it is almost there. So, two things:
- We tested it in the multi-year investment data, and it works. However, we want to cover another edge case outside that use case (so, in issue Create a meaningful year data for the Norse Case #684 I added that to the list to improve it). If we change the input to have the asset = ccgt in year = 2030 and commission_year = 2030 with investable = false, then it should not appear in the result of this query. As it is, it seems. Ni and I tried to fix it but couldn't (sorry about that). If we cover that case, then this change is okay.
- For the indices of the decommission variables, we don't need the investable column (we need the investment integer for creating the variable). So, we were wondering, why do you keep it? Maybe we are missing something. If we don't need it, then we should take it out.
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 add that to the actual tests? Otherwise there is guarantee that it works as expected.
I also don't see the relation between this new data row and the compact formulation. There are other rows that result ininvestable = false
in the query, soinvestable
is not taken into account -
I was just using for checking that things propagate
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 @abelsiqueira, thanks for the PR. We have a comment on the decommission variable indices. Please let us know if you need to discuss it, and we can comment on Teams.
5e581f4
to
69b58b9
Compare
Create field lookup to allow changing the variables without having to redefine the whole model creation Related to #923
69b58b9
to
2e224a4
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.
LGTM. Thanks!
Create field lookup to allow changing the variables without
having to redefine the whole model creation
Related to #923