GNOME Bugzilla – Bug 358236
Advanced Portfolio report miscalculates total return if money is moved between accounts
Last modified: 2018-06-29 21:13:13 UTC
Please describe the problem: The problem is that moving money from one account in the "Advanced Portfolio" report to another increments both money in and money out. The resulting gain is correct, but the total return uses an incorrect total money in value (and therefore is too low). Steps to reproduce: 1. Create two mutual fund accounts. 2. Buy $100 worth of shares of each one, at $1 a share. 3. Sell $50 of one of the funds and buy $50 of the other one in the same transaction. 4. Wait for the share price to rise to $2 a share. 5. Create an Advanced Portfolio report for the whole setup. Actual results: The "Total" line shows $400 for value, $250 for money in, $50 for money out, $200 for gain, and 80% for total return. Expected results: The "Total" line shows $400 for value, $200 for money in, $0 for money out, $200 for gain, and 100% for total return. Does this happen every time? Yes. Other information: I have a local hack to work around this where a transaction all of whose splits are for accounts that all have the same parent is ignored when calculating total money in/out. But this relies on my account tree structure, so is suboptimal. I can still post the diff if people are interested in. A better fix would be to ignore for the total money in/out transactions all of whose splits are for accounts that are part of the report. I haven't figured out yet how to do this; there's a bit of a dearth of documentation on a lot of the GnuCash Scheme-to-C stuff. ;)
Maybe the way to go is to just pass the whole account list as well as the "remaining accounts" list to table-add-stock-rows-internal.
Created attachment 73611 [details] [review] So something like this, maybe This sorta does what I suggested in comment 1. It's still suboptimal because it only really works "right" if one includes zero-share accounts in the display. But I'm not quite sure exactly what the expected behavior is when excluding those accounts (e.g. in terms of computing total money in and out), so I left that codepath alone.
Comment on attachment 73611 [details] [review] So something like this, maybe Oh, and parts of this feel really clunky to me. For example, I'd be surprised if there is not a cleaner way to check whether a given something is in the list using a given comparison function. But my Scheme is approximately 8 years rusty, and was never _that_ great...
Potentially related bugs (feel free to close this one or any of these as "duplicate" if they are in fact identical or a direct subset of this bug): bug#115267 bug#336240 bug#343245 bug#344566 bug#346062 bug#347739 bug#355660 Oh, and this was gnucash 1.8.x? Development on 1.8.x has stopped; please upgrade to 2.0.x and see whether this problem still occurs.
Oh, I completely forgot to mention the 2.0.x situation, sorry. Once I get it to compile (or once there is an RPM of it for Fedora Core 4) I will try it, but code inspection of all the changes to this file between 1.8.x and 2.0.x suggests that the problem will still be there. It may be hard to observe, however, depending on the exact effects of bug 355660 on my setup (that is the incorrectness due to that bug will probably just make things incorrect no matter whether this patch is applied). I agree that the patch is not useful as-is for 2.0.x; it's just there as a way of indicating what I'm trying to get GnuCash to do...
I know this bug is a bit old. I think, without digging into it too much that this may be fixed in r16620. Perhaps the submitter would be willing to test and provide feedback?
Andrew, by "r16620" I assume you mean <http://svn.gnucash.org/trac/changeset/16620>? Unfortunately this diff doesn't apply to the advanced-portfolio.scm that I have here (still on GnuCash 1.8 for the reasons listed in comment 5). I'm not going to have time to pull and build a version of GnuCash to test with, unfortunately. That said, I assume that since it's your patch you do in fact have a patched build. Would you be willing to test it using the steps to reproduce from comment 0?
Boris, I played with your example a little bit. I had to make some guesses from your example -- I assumed you meant the price of *both* mutual funds rose to $2. The report, as of r16620 (and yes, I mean that change set), does give the results you expect, but only under certain circumstances. Specifically, you need Price Editor entries for *both* funds. With the last transaction in the first mutual fund being expressed in terms of the second mutual fund, the report will grab a price expressed in terms of that mutual fund. This is obviously a problem, but it is easily circumvented by making sure there are current prices entered in the Price Editor. I'm not really up on mutual fund transactions, but it seems to me that trading one mutual fund for another is *not* an accurate model of what *actually* happens. I realize that on brokerage statements, it may look this way, but what is actually happening (to my understanding, please correct me) is you are *selling* units and then *buying* units. That implies that some actual money exists in between those two actions even if a brokerage statement doesn't actually show it. *I* would model this by moving that money through some temporary account, effectively splitting that txn into two transactions. In that context, I disagree with your assessment that Money Out should be zero. Money Out is specific to the actual account the mutual fund is stored in and not some higher level brokerage account. As such, there is money coming out of the first mutual fund and it should be recorded. Continued comments appreciated.
Oh, I forgot to mention, that for some reason, in this scenario, the price is not getting handled properly without a price db entry. So there are clearly still some problems with the report (but I think we knew that anyway).
> I assumed you meant the price of *both* mutual funds rose to $2. Yes. > With the last transaction in the first mutual fund being expressed in terms of > the second mutual fund Actually, the last transaction price is in dollars. Thinking back about the setup I encountered this in, I guess there is also a dollar-denominated brokerage account; the first fund is sold and he proceeds placed in that brokerage account, then the second fund is bought with the money in the brokerage account. In any case, requiring price information on both funds is perfectly reasonable. > In that context, I disagree with your assessment that Money Out should be zero. Money in or out for each individual fund should not be zero, agreed, since that tracks the performance of each individual fund. But the total money out isn't just the sum of the individual money out lines, when all the accounts involved are being tracked by the "Advanced Portfolio" report... Put another way, I'm interested in the performance of the group of accounts as a whole, in addition to the performance of each individual account. Even with a single account there is somewhat of a problem. Consider an extreme situation. Instead of the steps in comment 0, think of these steps: 1. Create one mutual fund account and one cash denominated brokerage account. 2. Place $100 into the brokerage account. 2. Buy 100 worth of shares the mutual fund, at $1 a share, with the money in the brokerage account. 3. Sell the 100 of shares of the mutual fund at $1 a share, with the proceeds placed in the brokerage account. 4. Repeat step 2. 5. Wait for the share price of the mutual fun to rise to $2 a share. 6. Create an Advanced Portfolio report for the whole setup (both accounts). The point being that steps 3 and 4 are basically a no-op as far as anyone is concerned. As currently calculated over here, the mutual fund account value is $200, the money in is $200, the money out is $100, the gain is $100, and the return is 50%, whereas "really" the return is 100%. I would assume that the same is the case with your patch, right? I'm not really sure how to "fix" this case short of computing return using some much more complicated formula than a simple "total gain over some basis number" division... It really feels like this is the correct way to go; I'm just not sure what this formula should be. In this same example, the "Total" row has $200 for the value, $400 for the money in, $300 for money out, and a return of 25% over here. This behavior might be changed by your patch.
Okay, so, we've got some conflicting ideas here. Remember that the report (as written) only sees things from the perspective of the actual stock/mutual fund account and not from the perspective of some higher-level brokerage account. So, you have to ignore that, frankly. True, in your example, steps 3 and 4 are a no-op in terms of net value and overall net cash flow, but they are actual transactions. So they have to be counted. Putting $100 into a mutual fund twice (even if you take it out in between) is still putting $200 into that mutual fund. Just like in a bank statement for your checking account, if you deposit $100 today, withdraw $100 tomorrow and then deposit $100 the day after, the bank is going to record $200 total deposits and $100 total withdrawals and not some net number of $100 net deposits. And no, the return is not 100%, it is 50% based on your total lifetime involvement in that fund. I agree that this is a very "Dumb" calculation, but it is what it is. To get the 100% return, you need to be able to exclude the previous transactions and look at only the history of the currently held shares. That is a much more complicated issue and is beyond the scope of this report at this time. Note that there is an RFE out there to handle starting dates (#312049) and that would effectively handle this situation for you. That said, I think it would be good to have a better report. And I think it's do-able, but it's no small feat. What would be a big help is to file an RFE bug that spells out clearly what kind of stock portfolio report you'd like to see. That way this conversation can be continued in a more relevant framework. Also, the current version of this report handles the Total line properly. Finally, this bug is against a version of the report that has been dramatically changed. I'm going to close this bug as Obsolete. That doesn't mean that I don't care about your issues, just that this bug is not relevant in the current context of development. I encourage you to continue this discussion elsewhere (maybe on -devel or -user, with me directly, or in a new RFE bug as discussed above).
GnuCash bug tracking has moved to a new Bugzilla host. This bug has been copied to https://bugs.gnucash.org/show_bug.cgi?id=358236. Please update any external references or bookmarks.