Skip to content

Conversation

Evernight
Copy link
Contributor

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.

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.

1 participant