Skip to content
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

Fix issue with fee computation #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jamesplease
Copy link

@jamesplease jamesplease commented Dec 12, 2017

I believe that the fees calculation for non-balanced portfolios is incorrect.

tl;dr, the fee is too low because the spending is deducted twice from the initial value before the fee is calculated.

This is the code fees calculation for non-rebalanced portfolios (link):

var feesIncurred = this.roundTwoDecimals((this.sim[i][j].portfolio.start - this.sim[i][j].spending + this.sim[i][j].equities.growth + this.sim[i][j].bonds.growth + this.sim[i][j].cash.growth + this.sim[i][j].gold.growth) * (form.portfolio.percentFees / 100));

The important part here is this part: this.sim[i][j].portfolio.start - this.sim[i][j].spending.

We can see that the spending is deducted from the portfolio start. This happens in the method calcEndPortfolio. If we look at the loop that computes the "cycle," we see that calcEndPortfolio is called after calcMarketGains. This is important because calcMarketGains has already subtracted the spending!

This means that we subtract the spending two times when calculating the fee. This does not affect rebalanced portfolios. Commit f915007 removed subtracting spending for rebalanced portfolios to fix a bug, but it's not clear to me if leaving this in for non-rebalanced portfolios was on oversight or a separate issue (it's been a long time since that commit was added).

An easy way to verify this is the following:

  1. Set constant growth of the portfolio: 0%
  2. Set fees/drag to 0%
  3. Set your portfolio to some amount (I used 625k)
  4. Set withdrawal to some amount (I used 25k)
  5. Put a breakpoint on the line that computes the fee, or console.log the value of this.sim[i][j].portfolio.start

In this situation, I would expect the first cycle to compute fees off of 600k, but it computes it off of 575k.

Because this is computing fees that are too low, non-rebalancing portfolios are producing results that are too optimistic.


This change allows for the calcEndPortfolio function to be written in a simpler way,
something like this (code not tested; just whipped that up quickly), but I did not add that refactoring to this PR to keep it clean.

Thanks for reading!

@boknows
Copy link
Owner

boknows commented Dec 20, 2017

I'm just seeing this now, as I've been busy at work. I'll review this hopefully by the new year. If this is correct, thanks for pointing it out!

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