New turnover, refactor fixed slippage adjustments

[gusgordon]

The new get_turnover method is different in a few ways:

We no longer support the average argument. This led to confusion. We also no longer support passing a period, because this was unused and also can lead to confusion. For example, if we passed period='1D', this is different than passing no period, because period='1D' would include weekends.

The denominator is now a function of actual gross book, or AGB. AGB is effectively the GMV from the algo's frame of reference, or longs + abs(shorts) EOD each day. Note, confusingly, this is different from an algo's GMV from the reference frame of a portfolio of algos, which is why we refer to it as AGB.

The denominator isn't AGB, but rather (AGB_t-1 + AGB_t) / 2). On day 1, we consider AGB_t-1 to be zero. (@marketneutral is this correct?)

The "default" is now average=False, whereas before the default was average=True. Again, this parameter has been removed.

Effectively, this means that avg_new_turnover ~= avg_old_turnover * 2 / avg_gross_lev if my math is correct. Approximate because of the averaging of the AGB.

Also, the fixed slippage adjustment functions have been refactored to not use the turnover function.

[gusgordon]

Talked to @marketneutral and we agreed we should also preserve the old behavior. I'll add an argument like denom, so people can pass denom='portfolio_value' if they want, if that makes sense you all. It should also be an argument on tear sheets so the plots can get generated using people's preferred way. average will still be gone, though.

[twiecki]

Note, confusingly, this is different from an algo's GMV from the reference frame of a portfolio of algos, which is why we refer to it as AGB.

Why?

The denominator isn't AGB, but rather (AGB_t-1 + AGB_t) / 2). On day 1, we consider AGB_t-1 to be zero. (@marketneutral is this correct?)

Why?

Looks good otherwise. Also needs engineering review @richafrank.

[gusgordon]

@twiecki When we talk about GMV, we have been using that to refer to the allocation to the algorithm (from the context of portfolio construction). If we have $100 and allocate evenly across 5 algos, each algo will have $20 GMV, regardless of if that algo is fully invested in the market or not. That's how we've been referring to GMV most often. The term AGB is also correct and avoids confusion — Jonathan recommended I do this.

We use average AGB on both EOD's wrapping around the transactions mainly because it helps us avoid NaNs. For example, if we an algo has $10 fully invested Tuesday EOD, then Weds it closes all positions, what is its turnover Weds? Using straight AGB it would be infinity, but using the average it would be $10 / (($10 + 0) / 2) = 200%. This was also at the recommendation of Jonathan.

Does that make sense?

[eigenfoo]

I find it confusing that in the doc string for get_turnover we basically say "we divide by GMV, but GMV is just AGB, and AGB is longs + abs(shorts) at EOD". That's a lot of new acronyms. Perhaps we should erase GMV from pyfolio altogether? That would clarify terminology. AGB for individual algos, for pyfolio users; GMV for portfolio of algos, for Q internally. Would that help?

We should probably also doc string the explanation about averaging yesterday's and today's AGB for the denominator.

[gusgordon]

@eigenfoo We don't use GMV anywhere in pyfolio, people are just familiar with the term gross market value; AGB is less known from my understanding. The explanation is AGB == GMV from the frame of the algo. And yeah, good idea, I clarified that a bit.

@twiecki test_tears started failing for Fama-French, because NaNs were being passed to sklearn's regression function which was causing an error. I just fixed it by dropping NaNs in the returns, I think this is the correct fix. I don't think we want to fillna(0), because that would change the regression results.

@richafrank Should be all set for you to review now, thanks.

[dmichalowicz]

@gusgordon Mostly looks great to me, I just had a few stylistic notes and a suggestion for one of the tests.

[dmichalowicz]

I'd write this as something like:

raise ValueError(
    "Unexpected value for denominator '{}'. The denominator parameter "
    "must be either 'AGB' or 'portfolio_value'.".format(denominator)
)

[dmichalowicz]

Could you expand a bit on this comment, maybe just explaining more (like you mentioned in this PR) that on day 1, we consider AGB_t-1 to be zero, so this is simply the first AGB value. This confused me at first without referencing back to your formula.

[dmichalowicz]

Could you add a brief comment explaining the 4.0 here? (I know why it is, just for future readers)

[dmichalowicz]

A couple comments on this test:

• it's probably worth adding a test for the case where you pass denominator='portfolio_value'
• the positions DataFrame confuses me a bit here. If there are transactions for 2 different sids, shouldn't the positions DataFrame presumably have columns for 2 sids as well? Also worth noting that the positions column for sid 0 is set to always be 10.0, meaning that doesn't really capture the averaging behavior where you do AGB.rolling(2).mean(). Incremental or alternating positions values would remedy this.

[gusgordon]

Good idea, I'll add this, and change it to just 1 sid.

[dmichalowicz]

LGTM