search

boknows / cFIREsim-open

An attempt to re-write the popular retirement calculator (cFIREsim.com) in mainly javascript, and begin the journey toward open-source.
Apache License 2.0
152 stars 52 forks source link

Fix issue with fee computation #71

Open jamesplease opened 6 years ago

jamesplease commented 6 years ago

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 f9150072bd629f4 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 commented 6 years ago

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!