Skip to content

BUG Fix turnover calculation #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 13, 2015
Merged

BUG Fix turnover calculation #117

merged 2 commits into from
Aug 13, 2015

Conversation

humdings
Copy link
Contributor

@humdings humdings commented Aug 7, 2015

This fixed the incorrect turnover calculation in
plotting.plot_turnover.

Now the plot calls pos.get_turnover rather than calculating
the values.

This fixed the incorrect turnover calculation in
`plotting.plot_turnover`.

Now the plot calls `pos.get_turnover` rather than calculating
the values.
if period is not None:
traded_value = traded_value.resample(period, how='sum')
portfolio_value = portfolio_value.resample(period, how='mean')
turnover = traded_value / 2.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using constants like here it's useful to add a comment on how it came about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, the doc string above says it uses the average of buys & sells, that's where the 2.0 comes from.

A lot of definitions use min(buys, sells) / average portfolio value, but we only store the abs value of buys + sells so averaging makes sense.

Sent from my iPhone

On Aug 10, 2015, at 6:46 AM, Thomas Wiecki [email protected] wrote:

In pyfolio/pos.py:

 """
  • turnover = transactions_df.apply(
  •    lambda z: z.apply(
    
  •        lambda r: abs(r))).resample(period, 'sum').sum(axis=1)
    
  • portfolio_value = backtest_data_df.portfolio_value.resample(period, 'mean')
  • turnoverpct = turnover / portfolio_value
  • turnoverpct = turnoverpct.fillna(0)
  • return turnoverpct
  • traded_value = transactions.txn_volume
  • portfolio_value = positions.sum(axis=1)
  • if period is not None:
  •    traded_value = traded_value.resample(period, how='sum')
    
  •    portfolio_value = portfolio_value.resample(period, how='mean')
    
  • turnover = traded_value / 2.0
    When using constants like here it's useful to add a comment on how it came about.


Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Then I'd add something like # traded_value contains the summed value from buys and sells; this computes the average of both.

@twiecki
Copy link
Contributor

twiecki commented Aug 10, 2015

Looks great! While we're here, do you think we should add unit tests?

@twiecki
Copy link
Contributor

twiecki commented Aug 10, 2015

Related issue: #104

@humdings
Copy link
Contributor Author

Yes, we should definitely have unit tests, I'll get to that a bit later today.

Sent from my iPhone

On Aug 10, 2015, at 6:49 AM, Thomas Wiecki [email protected] wrote:

Looks great! While we're here, do you think we should add unit tests?


Reply to this email directly or view it on GitHub.

Inline comment about computing the average of the buys and sells.
@twiecki
Copy link
Contributor

twiecki commented Aug 13, 2015

Great. I'll merge this in now as it fixes a bug. We can add a unittest later.

twiecki added a commit that referenced this pull request Aug 13, 2015
@twiecki twiecki merged commit de24725 into master Aug 13, 2015
@twiecki twiecki deleted the fix-turnover branch August 13, 2015 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants