Fix TWR calculation when truncating to common date #101
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.
While testing my changes related to currencies I noticed some strange behaviour that ended up not related to them but also was present in the current upstream version.
I was comparing TWRs of the portfolio and price graph with the same underlying currency. They were coincinding as expected because portfolio didn't include any additional fees or other cash flows that would create a difference. When I added third unrelated currency, however, results changed suddenly and the line graphs started to diverge.
After some debugging and reading the code I realized it's because of the way common date logic is implemented. The problem is that (value - first_value) is not a correct transformation for the TWR when trimmng the series. Instead each value should be basically recalculated (re-normalized) from the new starting point. The cleanest way probably would be to get portfolio_values, extract common date and then recalculate all series starting form the common date. But in this case all calculations would run twice which obviously doesn't increase linear complexity but we wouldn't want to increase the constant too much either. So I ended up using normalization_method which is a bit dirty but gets the job done as well.
Used this chance to reduce some of the code duplication.
The test added is the one that would return incorrect result between these changes.
Also this fixes another bug: when adding account series I forgot to add them to the common date calculation.