electricitymaps / bloom-contrib

Making carbon footprint data available to everyone.
https://www.bloomclimate.com
MIT License
434 stars 104 forks source link

Consumer Price Index #382

Closed pierresegonne closed 4 years ago

pierresegonne commented 4 years ago

resolves #373

corradio commented 4 years ago

Hey @pierresegonne ,

Those files are unfortunately too big to be able to be integrated here. They will cause too much memory to be used on the phone. Furthermore, I think the purchase carbon model also need to be updated for the issue to be fixed right?

In the spirit of iterative lean coding, I would suggest first taking into account inflation as a global factor, and then, as a second step, correct the existing currency conversion system (I'm not sure how it currently works, but maybe it's already able to convert currencies in the past).

It seems a bit wild to me to embed all timeseries of all possible currency conversions, alongside all consumer price indices. There must be an api for this?

Finally, I think it would be best to actually start a document that describes what the changes would be, and validate it together, before directly jumping into an implementation. This would remove the frustration of doing a lot of work that might not make past the merge ;-)

martincollignon commented 4 years ago

Just pitching in as I'm the one who guided @pierresegonne on the user needs.

I must admit I didn't take into consideration the memory issue nor North, as I was mainly focused on Lighthouse.

I couldn't find an API.

pierresegonne commented 4 years ago

Hey @corradio,

Good point, I didn't really have that bigger picture in mind as I thought I was just solving a small issue/working on a small improvement.

Regarding the carbon model, yes probably, I shouldn't have tagged the PR as resolving the issue but rather in connection to the issue, Mads was supposed to then give an insight into what needs to be changed to make the whole system work.

Having an API could be a solution, due to my limited understanding of the whole Tomorrow system I don't know how difficult it would be to create a separate API for that but might be worth investigating/making a POC?

I'll be on Holidays until Friday but I can spend some time next week looking into it :)

corradio commented 4 years ago

What I suggest is that we write specifications of how this is supposed to work (i.e. a design doc) and then we can figure out the best way to implement it. Let's look at it next week?

pierresegonne commented 4 years ago

Just pushed yearly indices for v0 of consumer price index initiative

corradio commented 4 years ago

Great! Are you able to make sure it's used in the calculations? I also suggest removing the exchange rate conversion which won't be used in this iteration (sorry for the work already done)

pierresegonne commented 4 years ago

Ok, I have just removed the exchange rates. I have not checked yet how the code for the purchase model works, I'll make sure that the indices are used in the calculations tomorrow morning :)

pierresegonne commented 4 years ago

I have pushed a WIP to get an opinion on the structure of the function and some insights into accessing the data inside the yml file. I used what is done with footprint.yml as inspiration but am not sure if it is the right approach

pierresegonne commented 4 years ago

Pushed a proposal for V0. I am nevertheless not sure about how to access the data from import consumerPriceIndex from './consumerpriceindices.yml'. I have treated it as if consumerPriceIndex was an object with the same structure as shown in the yml file (based on the example of import footprints from './footprints.yml' but I couldn't test it nor find relevant examples or doc online.

pierresegonne commented 4 years ago

By the way it is written CPI correction should be applied on raw activity monetary amount, before any conversion to EUR. in the brief, while here the CPI correction is done after the conversion. But this actually does not matter, ex:

10AUD in 2019

After conversion 10AUD = 10/1.6467 EUR = 6.07 EUR and then CPI factor = 106.9 / 92.2 = 1.16, so the final corrected EUR amount is 6.07 * 1.16 = 7.04 EUR

Before conversion CPI factor = 1.16 (that does not change, it is currency agnostic) so corrected AUD amount 10 * 1.16 = 11.6 AUD converted to EUR 11.6 / 1.6467 EURD = 7.04 EUR

Tell me @martincollignon if I am missing something or if it is because of the VAT that needs to be taken into account at later stage (in that case, if VAT are everywhere in %, that shouldn't change anything as well, as for above)

pierresegonne commented 4 years ago

@corradio Could you check that the last modifications are correct ? Thanks :)

corradio commented 4 years ago

Checkin now. For the future, if your PR is ready and you want me to review it, just press the "re-request review" next to the reviewer. That way I get a notification and will look at it 👍