RMI-PACTA / PACTA_analysis

Run the PACTA analysis on EQ & CB portfolios
Other
25 stars 70 forks source link

PACTA erroneously incuding short positions #463

Open FrederickFa opened 3 years ago

FrederickFa commented 3 years ago

After running PACTA with the current code I found that some results on portfolio level showed negative values in plan_alloc_wt_tech_prod & plan_emission_factor. As I am using port_weight, this can only happen (in theory) if the underlying ALD has negative production values (rather unrealistic but you never know) or that we are including short positions (negative values for market_value / value_usd) in the analysis. I think the latter is the case.

This would be a bug in my opinion, as we dont want to include short positions as this would distort the analysis.

Here is a reprex comparing results from two projects, one of which used an older version (from early autumn last year) of the code and one the current version

library(r2dii.utils)
library(dplyr)

# results with old pacta code
bonds_results_old <- read_rds(
  fs::path(path_dropbox_2dii(),"/PortCheck_v2/10_Projects/mfm_v7/40_Results/Bonds_results_company.rda")
) %>% 
  filter(port_weight < 0) %>% 
  filter(year == 2020, scenario == "B2DS")

equity_results_old <- read_rds(
  fs::path(path_dropbox_2dii(),"/PortCheck_v2/10_Projects/mfm_v7/40_Results/Equity_results_company.rda")
) %>% 
  filter(port_weight < 0) %>% 
  filter(year == 2020, scenario == "B2DS")

bonds_results_old %>% 
  as_tibble() %>% 
  select(portfolio_name, company_name, port_weight, allocation_weight, ald_sector, technology ,plan_tech_prod, plan_alloc_wt_tech_prod) %>% 
  distinct() 

equity_results_old  %>% 
  as_tibble() %>% 
  select(portfolio_name, company_name, port_weight, allocation_weight, ald_sector, technology ,plan_tech_prod, plan_alloc_wt_tech_prod) %>% 
  distinct()

# results with new pacta code
bonds_results_new <- read_rds(
  fs::path(path_dropbox_2dii(),"/PortCheck_v2/10_Projects/mfm_v8/40_Results/Bonds_results_company.rda")
) %>% 
  filter(port_weight < 0) %>% 
  filter(year == 2020, scenario == "B2DS")

equity_results_new <- read_rds(
  fs::path(path_dropbox_2dii(),"/PortCheck_v2/10_Projects/mfm_v8/40_Results/Equity_results_company.rda")
) %>% 
  filter(port_weight < 0) %>% 
  filter(year == 2020, scenario == "B2DS")

bonds_results_new %>% 
  as_tibble() %>% 
  select(portfolio_name, company_name, port_weight, allocation_weight, ald_sector, technology ,plan_tech_prod, plan_alloc_wt_tech_prod) %>% 
  distinct() 

equity_results_new  %>% 
  as_tibble() %>% 
  select(portfolio_name, company_name, port_weight, allocation_weight, ald_sector, technology ,plan_tech_prod, plan_alloc_wt_tech_prod) %>% 
  distinct()

When comparing the two versions of the code (I still have a copy of it) I think I found the commit since when we are analysing short positions: https://github.com/2DegreesInvesting/PACTA_analysis/commit/7e125aad61baafd39bd3d7e7d608d400694fe4f7

More specific, this line was deleted in 3_run_analysis.R: port_eq <- port_eq %>% filter(port_weight > 1e-6)

I dont know the motivation why we had this filter in the first place, but it lead to the fact that we kicked out short positions from port_eq, as negative values are below 1e-6. Without this filter we include short positions since this commit.

This raises a couple of questions from my end:

FrederickFa commented 3 years ago

@cjyetman @jacobvjk tagging you here because I think most relevant for you.

cjyetman commented 3 years ago

thanks @FrederickFa! excellent details

@jacobvjk you seemed to have had very specific reasons for this in #310, can you advise?

jacobvjk commented 3 years ago

@cjyetman yes I did. My context at the time was from the EIOPA project, where such small weights still sometimes represented sizeable sums, when looking at the entire European insurance sector. We wanted to ensure the same holdings remain in the analysis regardless of how you slice and dice the input portfolios.

Of course, I did not anticipate that this was also the single place to ensure we do not take short positions into account. And I think it's a bad way to filter out short positions. It should at least take into account the market value as a criterion and b something like filter(port_weight >= 0) or filter(market_value >= 0) (depending on where we filter) not a positive number...

I am still not aware if there is supposed to be another part in the code that would kick out short positions, but either way aadding something like this would seem like a viable quick fix?

FrederickFa commented 3 years ago

I agree with @jacobvjk that in case we keep this kind of filter we should change it to the proposed format.

Question is still whether we do this for port_cb as well?

jacobvjk commented 3 years ago

Maybe a question for the pacta analysts? Maybe they know if we ever come across short bond positions? Though if we set the filter to >= 0, I don't see the harm in adding it? @FrederickFa

FrederickFa commented 3 years ago

I included cases in the reprex for short bond positions resulting in negative plan_alloc_wt_tech_prod, so this exists apparently.

Therefore I would also agree / suggest filtering both port_cb and port_eq.

FrederickFa commented 3 years ago

In case this is the solution we are going for, I would also propose to only filter position above zero market value.

Therefore only filter(port_weight > 0), and not filter(port_weight >= 0). While holdings with zero market values actually dont make sense, these sometimes show up in Lipper outputs if the original share of a portfolio is really small, resulting in zeros in the market value column. Including such cases would lead to plan_alloc_wt_tech_prod = 0, what doesnt make sense to include in general and what creates at least some issues in the MFM code.

jacobvjk commented 3 years ago

good point @FrederickFa totally agree

cjyetman commented 3 years ago

Thanks folks! Also in agreement 100%.

So next steps in my opinion...

  1. figure out the most appropriate place in the code/process to do this (I would prefer this happen as soon as possible when the portfolio data is input, otherwise we're just passing around data we're not gonna use which could be dangerous elsewhere, not to mention it might improve performance if we don't do that)
  2. I think we should ask the PACTA team if they are broadly in agreement with removing any holdings, bonds or equity, that are zero or less
jacobvjk commented 3 years ago

should we bring it to the next Pacta Cop? (since the bi weekly was just two days ago and this seems to be P4I only anyways)

cjyetman commented 3 years ago

also looping in @AlexAxthelm here in case they want to take the lead on guiding the discussion about this issue with the overall PACTA team on behalf of the PACTA tech team

cjyetman commented 3 years ago

PACTA team, in my interpretation, gave the green light for this in the meantime, though it still may be a debate/question in the future.

AlexAxthelm commented 3 years ago

Thanks for taking the lead on this @cjyetman. Are you still looking for an "official" confirmation from the team, or are we good to go now?

cjyetman commented 3 years ago

I believe it was "official" enough.

FrederickFa commented 2 years ago

Hi guys!

I just checked this issue and I think it hasn't be resolved so far. This is a super heavy bug (as the label also says...) and in cases a portfolio has short positions it leads to totally wrong PACTA results. However, it is super straight forward to resolve (as described above).

Can we bring this forward?

@Antoine-Lalechere @looreen

Best Freddy

jacobvjk commented 2 years ago

will also post it here, more is more in this case.. @AlexAxthelm https://dev.azure.com/2DegreesInvesting/2DegreesInvesting/_workitems/edit/661