Refactor risk and return models

[robertmartin8]

Currently, there is a lot of repeated code within risk_models.py and expected_returns.py.

Almost all of the functions therein take prices as inputs, before processing them into returns, with the following couple of lines repeated a lot.

    if not isinstance(prices, pd.DataFrame):
        warnings.warn("prices are not in a dataframe", RuntimeWarning)
        prices = pd.DataFrame(prices)
    daily_returns = prices.pct_change().dropna(how="all")

In the spirit of DRY, I'd like to refactor this without complicating the API. Haven't decided the best way of proceeding. I suppose I could put these lines into a function, but that would probably need to go in a separate file (not very elegant IMO).

[robertmartin8]

Revisiting this issue, I think there are two solutions I could use:

1. Put the whole code block into a function and have each risk model call that function.

Pros: reduces repetition.
Cons: a bit ugly because each function will pass prices strait to the format_prices function. Also, people implementing new risk models will probably have to call that function?

2. Refactor into a RiskModels class, then handle formatting in the constructor

Pros: probably the standard way of doing it
Cons: for people to use their own risk model, they may end up having to inherit etc – a bit more of a hassle. Also, import paths become longer, e.g risk_models.RiskModels.function rather than risk_models.function.

All said, I am probably leaning towards the first one, but would appreciate a second opinion.

[Paposss]

Hi Rob, thanks for the amazing work in this.

Might not be direclty related to this post -
When running risk_models.CovarianceShrinkage I get the error message "cov_matrix is not a dataframe or array". Whereas the other risk models work fine.

Are there constraints to the datasets when running CovarianceShrinkage, or should it be called differently?

Many thanks for your help.

[88d52bdba0366127fffca9dfa93895]

@Paposss Hi, could you post full error/traceback then it easy to debug, I took a look at risk_models code but cant find your error message so I believe that it's not a problem with this package. Also, if it's not relate to this issue, please open a new one.

@robertmartin8 Hi, IMO the second approach is preferred, like the class CovarianceShrinkage, user can use it as

from risk_models import RiskModels
RiskModels.function(...)

[robertmartin8]

@Paposss @88d52bdba0366127fffca9dfa93895

Actually the error message is from PyPortfolioOpt, in the EfficientFrontier constructor. This error is raised if the input data is not in the correct format – @Paposss could you please double check how you are passing the data?

CovarianceShrinkage calls functions from pandas, and as such it may be a bit more picky regarding the format.

[robertmartin8]

@88d52bdba0366127fffca9dfa93895

thanks for the feedback, there is currently a refactor PR that I am merging ASAP, so it should improve usability.

[schneiderfelipe]

Hi,

I like the first approach and I think that an "input valldator" would work nicely (something that both receives and returns a dataframe for prices, similar to check_array in sklearn.utils).

But I also like the second (object-oriented) approach because it seems very "global" (all models in the same place). I think both can exist at the same time, if there's a way of not repeating oneself. I have two suggestions in this regard:

1. What about doing something similar to the fit/predict interface of sklearn? This would require a reinterpretation of their approach, but it seems natural to me.
2. Have you considered "one-size-fits-all" functions? scipy does that all the time (see scipy.optimize.minimize: a function receives a method parameter and an if statement transparently runs the appropriate function underneath:

# I have no idea how this should be named!
def risk_matrix(prices, method="sample", *args, **kwargs):
    if method == "sample":
        return sample_covariance(prices, *args, **kwargs)
    elif ...:
        ...
    else:
        raise NotImplementedError(f"method {method} not implemented")

Any "global" solution (either object-oriented or scipy-like) makes it very easy to do backtests and compare many different approaches with little boilerplate code. Besides, there's an easy solution for allowing user-provided, custom methods: receiving a callable as parameter (the following was taken from the scipy codebase, but comments are mine):

def minimize(fun, x0, ...):
    ...
    # Lots of input validation
    ...
    if meth == '_custom':
        return method(fun, x0, args=args, jac=jac, hess=hess, hessp=hessp,
                      bounds=bounds, constraints=constraints,
                      callback=callback, **options)
    elif meth == 'nelder-mead':
        return _minimize_neldermead(fun, x0, args, callback, **options)
    elif meth == 'powell':
        return _minimize_powell(fun, x0, args, callback, **options)
    ...
    # Many more elif statemets
    ...

I personally think that this last possibility is a very elegant solution.

[robertmartin8]

@schneiderfelipe

Thanks for the suggestions! I'll have a think about this. I'm not a huge fan of the fit/predict interface because I don't think it makes intuitive much sense for covariance matrices.

Your second suggestion is very interesting, and probably how I'd do it if I decide to make any changes. I'm still trying to figure out whether it's worth it because it'd be a breaking change.

[robertmartin8]

Alternatively, I could just get rid of all of the input sanitisation and trust the user to enter a pandas dataframe...

[schneiderfelipe]

Thanks for the suggestions! I'll have a think about this. I'm not a huge fan of the fit/predict interface because I don't think it makes intuitive much sense for covariance matrices.

I totally agree :)

Anyway, a check_dataframe function could be useful.

Your second suggestion is very interesting, and probably how I'd do it if I decide to make any changes. I'm still trying to figure out whether it's worth it because it'd be a breaking change.

I think it doesn't need to be a breaking change. Both ways of using risk and return models can exist simultaneously, e.g., the following lines would be equivalent:

S = risk_models.semicovariance(prices)
S = risk_models.risk_matrix(prices, method="semicovariance")  # not sure about this name "risk_matrix"