Conversation
dalonsoa
left a comment
There was a problem hiding this comment.
I've left a couple of comments but, I think, they mostly boil down to docstrings, so approving. It has taken a couple of iterations, but I think the explanation makes sense.
| if data.timeslice.reset_coords(drop=True).equals( | ||
| ts.timeslice.reset_coords(drop=True) | ||
| ): |
There was a problem hiding this comment.
Why was this change needed?
There was a problem hiding this comment.
I had to modify this function (and others) to support timeslice arrays with multiple dimensions. This is so that different timeslice weights can be used for different technologies (important when splitting costs based on production). No exactly sure why reset_coords is required for this, but it was giving errors without this. There were a couple of other changes required like doing sum("timeslice") to sum the timeslice weights instead of just sum().
I've now modified the timeslice fixture in the tests so that both scenarios are tested - single dimensional and multi-dimensional timeslice arrays - all seems fine
| # Split fixed/variable across timeslices in proportion to production | ||
| timeslice_level = get_level(production) | ||
| tech_activity = production_amplitude(production, technologies) | ||
| _fixed_costs = distribute_timeslice( |
There was a problem hiding this comment.
This is an unexpected use of distribute_timeslice, I think.
According to its docstring, the ts parameter should be a Dataarray with timeslice lengths., but here you are giving tech_activity. I understand that you are using this as weights to distribute the costs, which makes sense, so maybe the problem is in the description of distribute_timeslice, which should clarify that ts are the weights assigned to each timeslice and not necessarily their lengths (in time units).
There was a problem hiding this comment.
Good point - ts refers to weights, not necessarily lengths. I've updated the docstring
|
|
||
| # Distribute capital costs across timeslices in proportion to production | ||
| tech_activity = production_amplitude(production, technologies) | ||
| _capital_costs = distribute_timeslice( |
There was a problem hiding this comment.
So the whole weighting by the tech activity also applies to the NPV, right?
There was a problem hiding this comment.
I assume so, yeah. We haven't talked about NPV as much, but if you don't do this it will artificially deflate NPV in timeslices with lower output. I think the expected behaviour is that NPV should be linear with production (assuming that commodity prices are equal in every timeslice). I've added a test for this.
However, we don't actually need timeslice-level NPV, as it's only used for the objectives, so it might make more sense to calculate it over the year as a whole, like I've done for LCOE in #635
Description
This PR follows on from #609 (which I reverted), and is more along the lines of #605 (which I initially rejected, but now favor)
The main issues are whether LCOE should be calculated at the timeslice-level or for the year as a whole, and ensuring this calculation is correct. When calculating LCOE for objectives, it makes more sense to have a single LCOE value for the year which depends on annual utilization. This is addressed in a separate PR (#635), which should be reviewed after this one.
However, it still makes sense to calculate LCOE at the timeslice-level when calculating commodity prices (see #613). LCOE is currently being calculated at the timeslice-level (and only ever at the timeslice-level), but the approach for doing this is wrong, which is what I'm trying to fix in this PR.
In particular, capital costs are being distributed evenly across timeslices (in proportion to timeslice-length), which artificially inflates LCOE (and therefore commodity prices) in timeslices with low output (as the capital cost per unit of output is higher). Realistically, the capital cost per unit of output should be the same in every timeslice, which means splitting costs in proportion to production rather than timeslice-length. The same is true for fixed costs and variable costs.
I've added a test which highlights the issue (
test_lcoe_equal_prices). If commodity prices are equal in every timeslice, then LCOE should also be equal in every timeslice. This was not the case previously, due to the issue about per-unit capital cost described above.Another outcome is that if production is zero in a timeslice, then all components of LCOE (fuel costs, capital costs etc.) will now be zero in that timeslice. In this case, LCOE is strictly undefined, but will end up being zero in the calculation due to this line. This is captured in
test_lcoe_zero_production. I think it's ok the LCOE gets set to zero, as timeslices with zero production will be excluded from the weighted average when calculating commodity prices. (And also fine for the objectives due to thetimeslice_maxoperation here, but this is changing anyway in #635 so I'll address this there).Smaller changes:
timeslice_levelto functions in thecostsmodule. Most functions don't need this, and those that do can infer the timeslice level from passed inputstimeslicesmodule to support multi-dimensional timeslice weights (thetsargument to functions). This was necessary for splitting capital costs over timeslices, as different technologies require different timeslice weights.Another important thing to remember is that operations between timesliced and non-timesliced arrays are prohibited. This precludes many potential timeslice-related calculation errors, such as accidentally applying the full annual capital cost to every timeslice.
Fixes #288 (partially, see also #635)
Fixes #613
Type of change
Key checklist
$ python -m pytest$ python -m sphinx -b html docs docs/buildFurther checks