CNugteren / DGPC

DGPC: DeGiro Performance Charts
MIT License
17 stars 2 forks source link

bank_cash and nominal value don't work as expected #2

Open erikwt opened 4 years ago

erikwt commented 4 years ago

Hey!

First off, thanks for open sourcing this work, nicely done! I was super lucky to run into this. I just started a very similar side-project and it looks like I can build a POC of what I have in mind on top of this. I had some trouble getting started because of parsing issues, and I submitted fixes through a PR. Also, some transaction types are not yet supported, like "Rente" and a few others. I think I'll add those at some point. I also plan to look into the currency conversion, because the one used now does not match what is paid through degiro, probably because the bid/ask spread. I do believe it's possible to derive it from the data though.

The bigger problem I see is that bank_cash and nominal value don't get calculated correctly, and from the code I also don't see how it's intended to work. If you have a degiro account where money goes in and out and back again, the data gets messed up. I was able to get the right data for my use case by using this as deposit code, and not using bank_cash at all.

if description in ("iDEAL storting", "Storting"):
    cash[date_index:] += mutation
    invested[date_index:] += mutation

elif description in ("Terugstorting",):
    cash[date_index:] += mutation
    invested[date_index:] += mutation

I can submit it in a PR, but I'll hold off to see if you respond with some more info about the intention.

Before the changes dgpc-storting-fix

After the changes (this is closer to the correct representation). Ignore the profit/loss line, that's unrelated. Also note that you can see that the performance is number is not valid anymore when you pull money out of the account, because your (unrelized) gain is now relative to a smaller number. dgpc-fixed

Again, thanks for open sourcing and let me know if you have questions or remarks about the above. Cheers, Erik

CNugteren commented 4 years ago

First off, thanks for open sourcing this work, nicely done! I was super lucky to run into this. I just started a very similar side-project and it looks like I can build a POC of what I have in mind on top of this.

Nice to hear. Do share a link to it here when it is ready :-)

I had some trouble getting started because of parsing issues, and I submitted fixes through a PR. Also, some transaction types are not yet supported, like "Rente" and a few others. I think I'll add those at some point.

Thanks, I saw them. Yes, everything that is currently supported is based on what I've seen in my own account. I didn't have any other data when I wrote it, but I hope I made it such that it shouldn't be too difficult to add other types.

I also plan to look into the currency conversion, because the one used now does not match what is paid through degiro, probably because the bid/ask spread. I do believe it's possible to derive it from the data though.

I think one of the reasons it doesn't match is that I base it on the final value of that day, and not on a particular hour/minute moment on that day. I'm not sure if that data is easily available, but I thought this would be approximate enough to generate useful graphs.

The bigger problem I see is that bank_cash and nominal value don't get calculated correctly

I admit the bank-cash idea might not fit everyone's use case and is something we could also consider removing entirely indeed. My philosophy was that booking money out is never going to happen - unless you temporary put it on your own bank-account to get a higher interest. Eventually that money will go back to the DeGiro account and stock can be bought again. This is of course not true but it did fit my scenario. Do you think there is a bug with the bank-cash implementation or is it just that it doesn't fit your scenario? You should see the red line as DeGiro cash + bank cash.

So I'm OK with removing this concept as you suggested in the code, but now we get issues with the bottom graph showing the relative figures and the benchmarks being less meaningful. Perhaps we can keep the concept of bank-cash for the purposes of the benchmark graphs but just deduct the value from some of the curves in the top graph?

erikwt commented 4 years ago

I think calculating performance like that only works if there is no in- or outflow of cash. Using something like Time-Weighted Return would be much more accurate and meaningful imo. Would you consider adding something like that? If not I will add it to my backlog, but I'm not sure if I will get to it in the near term as my focus is more on the data visualisation of the absolute values at this point.

I'm also curious - just in general - if you plan to work more on this project. At some point in the future I think I'll either try to use your project as a library (the parsing part), or move away from it altogether after incorporating the parser. I don't know yet when it makes sense to do so, as it's obviously beneficial if we make use of each others improvements. Any thoughts?

I want to say thanks again by the way, I can't believe how lucky I was to find your project - which was started so recently but already works so well! My project, which is a web project based around visualizing the portfolio data, will be open source as well.

CNugteren commented 4 years ago

Using something like Time-Weighted Return would be much more accurate and meaningful imo

Yes I guess that makes sense to do. I could see if it is not that difficult to do. I hope that it doesn't change any numbers if you compare it to the current implementation with the bank-cash concept. However, that still leaves the benchmark ETF graph, which is currently exactly as I want it to be for my use-case as well. What to do there with withdrawals?

if you plan to work more on this project

Currently it does everything for my use-case, so I don't plan to work actively without specific requests. I will of course look at pull-requests, but don't expect me to contribute too much. If you want, I could also make you a contributor/owner of this project?

as it's obviously beneficial if we make use of each others improvements. Any thoughts?

This is still a small project with few users written in a few hours, so I'm not sure we have to go through great lengths to do the very best possible. I would also be OK if you just use the parser separately in your own project and not propagate updates to DGPC, if that gives you more freedom?

CNugteren commented 4 years ago

I found some time and implemented the TWR computation according to your link, removed the bank cash concept, and reorganized some code a bit. You can find the results here: https://github.com/CNugteren/DGPC/pull/3

However, I'm not sure it is correct, so please have a look. In the example graphs the difference is not so big (but still there is something) between the actual returns: new_graph

I don't have a background in finance, so I don't understand why the performance can't be calculated by dividing the total account value by the money you've put in at a given time, which is what I used before to compute the account performance graph. Also in the example from the link you gave, I would say not 21.49% gain, but 21.88% gain: (25992 - 1500 + 250) / 20300.

The values I have plotted now for my own data (not shared here) according to the TWR computation would indicate I had at one point a 50% return (which I understand as e.g. 20K money put in and current value at 30K - but this didn't happen) and never had a return below 0%, but I did have less money in my account than invested this March. So either my calculation/understanding is wrong, or the TWR is a value that doesn't tell me much :-)

erikwt commented 4 years ago

That's awesome! I'm going to check it out validate the calculations. I'll post a more elaborate response later.