Skip to content
This repository has been archived by the owner on Dec 14, 2023. It is now read-only.

PACTA erroneously incuding short positions #463

Open
FrederickFa opened this issue Jun 9, 2021 · 16 comments
Open

PACTA erroneously incuding short positions #463

FrederickFa opened this issue Jun 9, 2021 · 16 comments
Labels
bug Something isn't working high-priority

Comments

@FrederickFa
Copy link

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: 7e125aa

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:

  • Do we intend kickout short positions at a different part of the code?
  • If not, how should we kickout short positions in the investor code?
  • Why do we only excluded short position in port_eq and not also in port_cb? It is possible to have negative bonds as well.
@FrederickFa FrederickFa added the bug Something isn't working label Jun 9, 2021
@FrederickFa
Copy link
Author

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

@cjyetman
Copy link
Member

cjyetman commented Jun 9, 2021

thanks @FrederickFa! excellent details

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

@jacobvjk
Copy link
Member

jacobvjk commented Jun 9, 2021

@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
Copy link
Author

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
Copy link
Member

jacobvjk commented Jun 9, 2021

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

jacobvjk commented Jun 9, 2021

good point @FrederickFa totally agree

@cjyetman
Copy link
Member

cjyetman commented Jun 9, 2021

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
Copy link
Member

jacobvjk commented Jun 9, 2021

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
Copy link
Member

cjyetman commented Jun 9, 2021

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

I believe it was "official" enough.

@FrederickFa
Copy link
Author

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
Copy link
Member

jacobvjk commented Oct 8, 2021

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working high-priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants