filter out short and null holdings

[cjyetman]

related to ADO-661
supersedes #499

This is @FrederickFa's suggested (quick but not ideal) change to address the problem discussed in #499. This is also discussed in ADO#661, where there seems to be still some debate on the proper long term solution.

This only changes the so-called "offline" version, which may be only used by MFM anymore, though we can't really know that for sure. This will also impact what runs on transitionmonitor

[FrederickFa]

Looks good to me!

Also, we actually just go back to an earlier version of PACTA (7e125aa) but filter holdings of zero portfolio weight too and also repeat this process for CBs.

[jdhoffa]

lgtm

[cjyetman]

thanks @FrederickFa and @jdhoffa... I think we need to wait for @AlexAxthelm's approval on this one though

[AlexAxthelm]

I'll admit that I'm out of my depth on the methodological impact of this change, but conceptually, it makes sense.

[cjyetman]

putting back into draft until a final decision is made in ADO-661

[jdhoffa]

@cjyetman who else needs to be consulted regarding this decision?

[cjyetman]

@jdhoffa according to the ADO thread, I suppose Maarten and Nayra

[jdhoffa]

I believe Maarten's feedback was as follows:
"Since we do not have a methodological position yet, for now in the short term, negative ISINs should not be allowed to be uploaded. This means they are never part of the calculation of exposures (or should not be), and never part of the analysis. If that is the case (ie if our audit kicks them out as invalid securities), then this is not an urgent problem for the PACTA team."

[jdhoffa]

also hi! hope you guys had nice holidays hahaha

[FrederickFa]

I believe Maarten's feedback was as follows:
"Since we do not have a methodological position yet, for now in the short term, negative ISINs should not be allowed to be uploaded. This means they are never part of the calculation of exposures (or should not be), and never part of the analysis. If that is the case (ie if our audit kicks them out as invalid securities), then this is not an urgent problem for the PACTA team."

I also understood it that way. But since they are currently still integrated into the analysis, we should filter them out with the simple solution proposed here.

[cjyetman]

I believe Maarten's feedback was as follows:
"Since we do not have a methodological position yet, for now in the short term, negative ISINs should not be allowed to be uploaded. This means they are never part of the calculation of exposures (or should not be), and never part of the analysis. If that is the case (ie if our audit kicks them out as invalid securities), then this is not an urgent problem for the PACTA team."

That's not what this PR does. It does not prevent uploading negative ISINs, and it does not flag them as invalid in the audit file. The filter in this PR occurs long after the audit file is created.

[jdhoffa]

I believe Maarten's feedback was as follows:
"Since we do not have a methodological position yet, for now in the short term, negative ISINs should not be allowed to be uploaded. This means they are never part of the calculation of exposures (or should not be), and never part of the analysis. If that is the case (ie if our audit kicks them out as invalid securities), then this is not an urgent problem for the PACTA team."

That's not what this PR does. It does not prevent uploading negative ISINs, and it does not flag them as invalid in the audit file. The filter in this PR occurs long after the audit file is created.

That makes sense, but we don't need further input from Maarten or Nayra do we? We need to filter out these positions somewhere in 2_project_input_analysis.R (prior to the analysis being run). And I guess potentially somewhere in web_tool_script_2.R(?) unless this already happens for the web tool on portfolio upload.

[cjyetman]

That's what I did before and @FrederickFa said it was wrong... #499

[jdhoffa]

That's what I did before and @FrederickFa said it was wrong... #499

Aw barnacles, ok...

[FrederickFa]

I believe Maarten's feedback was as follows:
"Since we do not have a methodological position yet, for now in the short term, negative ISINs should not be allowed to be uploaded. This means they are never part of the calculation of exposures (or should not be), and never part of the analysis. If that is the case (ie if our audit kicks them out as invalid securities), then this is not an urgent problem for the PACTA team."

That's not what this PR does. It does not prevent uploading negative ISINs, and it does not flag them as invalid in the audit file. The filter in this PR occurs long after the audit file is created.

Yes you are right this is not exactly what this PR does. It does make sure they are not part of the analysis and also maybe that these positions are not included when calculating exposures (I am not sure at what step this happens). It does not make sure they are not uploaded or filtered right away.

In terms of flagging, this is already happening, which is why it is so confusing. We already add flags that these are not valid input values (lines 666 & 705) but do include them in the analysis later on.

https://github.com/2DegreesInvesting/PACTA_analysis/blob/b4e4846688bb1a6ce37434b14f31ee4d3fcd097a/0_portfolio_input_check_functions.R#L660-L673

https://github.com/2DegreesInvesting/PACTA_analysis/blob/b4e4846688bb1a6ce37434b14f31ee4d3fcd097a/0_portfolio_input_check_functions.R#L701-L713

---

That's what I did before and @FrederickFa said it was wrong... #499

Well, there is no right or wrong, but I think the aspects mentioned in #499 are still relevant. Since we do not yet have a methodological position, I think it is better to intervene as little as possible in the portfolios, which for me means not analysing short positions for the time being but leaving everything else untouched.

Argument in #499:

Why? Because otherwise we change the weights of positions in the portfolio. As short positions have negative values, filtering them would increase the portfolio size and thus also decrease the weight of single positions. This would change the sector exposure later on.

If the general consensus is to filter them out of the portfolio immediately, that is also a valid solution. We should just make sure that we move forward with one solution or the other and not leave it as it is.

[FrederickFa]

I was just thinking and maybe we should go even further and have a filter like this:

port_eq %>% dplyr::filter(has_valid_input == TRUE)
port_cb %>% dplyr::filter(has_valid_input == TRUE)

instead of:

port_eq %>% dplyr::filter(port_weight > 0)
port_cb %>% dplyr::filter(port_weight > 0)

---

For example, if a user would upload a portfolio which holdings have negative values in number of shares (which I dont think makes sense at all but can happen because of a mistake), then these positions would (probably) be analysed because we apparently do not have a filter in place. Hence, it think it makes sense to make sure only positions get analysed later on which are valid. And within check_valid_input_value() we should specify what we consider to be a valid input.

https://github.com/2DegreesInvesting/PACTA_analysis/blob/b4e4846688bb1a6ce37434b14f31ee4d3fcd097a/0_portfolio_input_check_functions.R#L660-L673

[jdhoffa]

This seems stale, and also in a deprecated repo. Ok to Close?

[cjyetman]

actually, I think we never came to an agreement on how to deal with this.... probably should still consider it

[jdhoffa]

actually, I think we never came to an agreement on how to deal with this.... probably should still consider it

Maybe then we:

• close this PR
• make a new issue in pacta.portfolio.analysis (tagging this PR for the content in the conversation
• maybe tag as priority?

[jdhoffa]

Just trying to do some house-keeping

[cjyetman]

actually, I think we never came to an agreement on how to deal with this.... probably should still consider it

Maybe then we:

• close this PR
• make a new issue in pacta.portfolio.analysis (tagging this PR for the content in the conversation
• maybe tag as priority?

💯 agree with that

[jdhoffa]

I can't close this PR because I dont have admin to this repo anymore, but the rest is done.

[cjyetman]

closing but tracking issue here https://github.com/RMI-PACTA/pacta.portfolio.analysis/issues/177