RMI-PACTA / pacta.portfolio.report

pacta.portfolio.report
https://rmi-pacta.github.io/pacta.portfolio.report/
MIT License
1 stars 0 forks source link

change filtering to keep companies where plan_tech_share is 0 #68

Closed Antoine-Lalechere closed 4 months ago

Antoine-Lalechere commented 4 months ago

related to https://dev.azure.com/RMI-PACTA/2DegreesInvesting/_sprints/taskboard/P4I/2DegreesInvesting/2024_09?workitem=8407

This PR aims to keep in the bubble company chart the company with no green technology, i.e. company where plan_tech_share for green technology is 0.

cjyetman commented 4 months ago

I reviewed this as extensively as possible with @Antoine-Lalechere. My suspicion is that the filter for plan_tech_share != 0 was to avoid any possible division by zero which could easily lead to an error, however @Antoine-Lalechere and @cjyetman don't see any reason why that value would end up as the numerator for any calculation. So, I am approving this, but I think @MonikaFu will still have to approve because they are the code owner.

MonikaFu commented 4 months ago

Thanks @cjyetman. I'll review tomorrow unless it's urgently needed.

cjyetman commented 4 months ago

Looks good to me on paper but when I run it locally I did not get the company bubble to show up in the report and the data file for this chart was an empty array. Not sure if it is maybe an issue with the data I'm using or the code. @cjyetman @Antoine-Lalechere any ideas? Have you tested this locally and looked at the plot?

Do you have an appropriate portfolio CSV to test it, i.e. one that has an ISIN that points to a Power company that is pure brown and has 0 production in the start year? We had difficulty finding one, so I relied on logic and reviewing the long history of this function/pipe (the filter has been there since the very beginning, like 4 years ago).

MonikaFu commented 4 months ago

@cjyetman no, I don't, I wanted first to see if the report generates and then look into editing data for this particular chart. But it seems like there is a mismatch again between data I am using and scenario names :/

MonikaFu commented 4 months ago

sorry @cjyetman I accidentally re-requested a review from you..