Move some of the active checks earlier in the workflow #962
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #962 +/- ##
=======================================
Coverage 95.45% 95.45%
=======================================
Files 29 29
Lines 1034 1034
=======================================
Hits 987 987
Misses 47 47 ☔ View full report in Codecov by Sentry. |
| LEFT JOIN asset_both | ||
| ON asset_both.asset = t_union.asset | ||
| AND asset_both.milestone_year = t_union.year | ||
| AND asset_both.commission_year = t_union.year | ||
| LEFT JOIN rep_periods_data | ||
| ON t_union.year = rep_periods_data.year | ||
| AND t_union.rep_period = rep_periods_data.rep_period | ||
| ORDER BY asset, t_union.year, t_union.rep_period, time_block_start | ||
| WHERE asset_both.active = true |
There was a problem hiding this comment.
Just for my understanding, the JOIN here is to have available in the table the active parameter. So, when we take out active from the Tulipa files in #960 we don't have to do this JOIN. Is that correct?
There was a problem hiding this comment.
The stored table itself won't have the active parameter because I haven't selected it, but the intermediary table resulting from the join will, which is then filtered in this WHERE. So the JOIN is only so that we have active available for filtering.
There was a problem hiding this comment.
So, deleting the active also will also delete this extra join and make the code more simple 😄
Thanks!
There was a problem hiding this comment.
During #960, this will also be removed as there will be no mention of active anywhere. I was thinking about where the removal of active will happen, and the earliest point would be not having active at all in the input of TulipaEnergyModel. That means that during the workflow processing, at some point we check for active and remove. In the current workflow, it would be at the read_csv_folder function in TulipaIO. It's not clear yet the implications, and it's not urgent, so I'm not actively thinking about it.
For this PR, I noticed the constraint creations had a bunch of active checks, and the constraints could look a bit simpler if they didn't, so I shifted left.
There was a problem hiding this comment.
Hi, @abelsiqueira; it looks cleaner and simpler to have it in the calculation of the highest-resolution table. Thanks! This will also simplify the work in #960
|
Thanks for the review! |
This doesn't remove the active as proposed in #960, but it moves the check to earlier in the workflow - to the highest and lowest resolution.
Relates to #960