Conversation
c1b17cf to
1234144
Compare
dalonsoa
left a comment
There was a problem hiding this comment.
It looks OK. The code is cleaner and with better explanations. Having said that, I've a few questions/doubts.
| result = result.where(result >= expanded_minprod, expanded_minprod) | ||
|
|
||
| # add production of environmental pollutants | ||
| # Multi-region models |
There was a problem hiding this comment.
Ok, I got a bit confused here. What's the point of multi-region simulations if there's no trade between them? You could equally have individual simulations for each of the regions as they are, in practice, independent from each other, right?
For some reason, I had always implicitly assumed that if you have multiple regions, you're going to send stuff from one region to another. I never consider that that was not required.
There was a problem hiding this comment.
Good point. I agree, it's pointless without trade
However, trade requires things to be set up in a particular way, with a TradeTechnodata.csv file, and it's still possible to run a multi-region model without this file, which we need to account for (at least for now).
The multi-region model in the tutorials doesn't have any trade, and is just used to demonstrate how regions with different technology parameters can differ - although obviously you could do the same with two single-region models!
| This function broadcast the first representation to the shape and coordinates | ||
| of the second. | ||
|
|
||
| Note: this is not necessarily limited to `technology` datasets. For |
There was a problem hiding this comment.
Since this function is not limited to technology datasets, it should be renamed to something different as well as well as use variable names that do not indicate the type of data. Maybe broadcast_data for the function name, and just data or dataset instead of technologies. The docstring and comments will need to be updated accordingly.
| applied to the `year` dimension of the technologies dataset | ||
| kwargs: further arguments are used initial filters over the | ||
| `technologies` dataset. | ||
| installed_as_year: True means that the "year" dimension in the technologies |
There was a problem hiding this comment.
It might be worth to add an explanation of the use case for this to be False, as well.
| # If installed_as_year is True, we need to rename the installed dimension to "year" | ||
| # TODO: this should be stricter, and enforce that the template has "installed" data, | ||
| # and that the technologies dataset has a "year" dimension. | ||
| # if installed_as_year: |
There was a problem hiding this comment.
As there seem to be more things to do, maybe implement the changes I suggest above as a separate PR, alongside this TODO.
70ed464 to
cde04f8
Compare
To be reviewed after #647
The main changes in the PR are a rewrite of the
supplyandemissionfunctions, along with some changes tobroadcast_techssupplydemandhas a "region" dimension, so the lineexpanded_demand = (demand * maxprod / maxprod.sum(demsum)).fillna(0)created an array with separate "asset" and "region" dimensions, which essentially duplicated each asset over every region (this is why regression tests are failing in Xarray patch: Check array dimensions #647). This led to the problem described in [BUG] Unexpected behaviour in multi-region models #475, and also inflated commodity prices in multi-region models (see Prevent arrays from having separate "asset" and "region" dimensions #619). I've fixed this by callingbroadcast_techson thedemanddata so that each asset only sees the demand for its respective regionemissionproduction_amplitude), rather than the sum of end-use quantities as was being done before. This is especially relevant for technologies with multiple end-use commodities, or output ratios not equal to 1.broadcast_techsinstalled_as_yearparameter, as this is actually very important and care needs to be taken depending on the type of data that the function is called on.kwargsas this wasn't being used anywhereResults
Power_Supply.csvfiles now have data for 2020 which was missing before - not sure why, but I'll take itClose #475