Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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):
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 thatcalcEndPortfolio
is called aftercalcMarketGains
. This is important becausecalcMarketGains
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:
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!