search

hoostus / portfolio-returns

Beancount portfolio returns generator
Other
38 stars 6 forks source link

Stock conversions cause incorrect output #3

Closed redstreet closed 2 years ago

redstreet commented 4 years ago

Example below involves a stock conversion (eg: a stock split), which should make no change to inflows, outflows, or cashflows. Expected output: 50% Actual output: 150%

option "operating_currency" "USD"
plugin "beancount.plugins.auto_accounts"

2018-01-01 * "Buy"
   Assets:Investments 100 HOOLI {1 USD}
   Assets:Bank

2018-04-01 * "Conversion"
   Assets:Investments -100 HOOLI {}
   Assets:Investments 50 IOOLI {2 USD}

2018-12-31 price HOOLI 1.5 USD
2018-12-31 price IOOLI 3 USD

irr.py --account Assets:Investments test.bc --from 2018-01-01 --to 2018-12-31```
hoostus commented 4 years ago

Thanks for the report, I'll look into it over the weekend.

hoostus commented 4 years ago

The problem is that the call to beancount.core.convert.convert_position() is (essentially) failing on the 50 IOOLI line. The reason it fails is because there's no IOOLI in the pricemap at that point in time.

This should work, so maybe there's another way I can get the value of a position from beancount. I'll have to research and see.

In the meantime, the workaround is pretty easy: you just need to add a price directive for IOOLI on or before 2018-04-01:

2018-04-01 price IOOLI 2 USD

redstreet commented 4 years ago

Hmm, I still get incorrect results. Let me post a few test cases below. In all these test cases, the expected output is 50%.

All these involve a stock conversion, where no cash is transacted. This means that the cost basis of the new stock is based on the cost basis of the existing stock, and is unrelated to the price on that day. Eg:

2018-01-01 * "Buy"
   Assets:Investments 100 HOOLI {2 USD}
   Assets:Bank

2018-04-01 price IOOLI 3.50 USD
2018-04-01 price IOOLI 7 USD

2018-04-01 * "Conversion"
   Assets:Investments -100 HOOLI {}
   Assets:Investments 50 IOOLI {4 USD} ; Cost basis of IOOLI is 4 USD, even though current market price is $7 USD, because this is a stock conversion from my existing HOOLI, whose cost basis is 2 USD.
redstreet commented 4 years ago

Case 1: This is the case you identified above.

option "operating_currency" "USD"
plugin "beancount.plugins.auto_accounts"

2018-01-01 * "Buy"
   Assets:Investments 100 HOOLI {2 USD}
   Assets:Bank

2018-04-01 * "Conversion"
   Assets:Investments -100 HOOLI {}
   Assets:Investments 50 IOOLI {4 USD}

2018-12-31 price HOOLI 3 USD
2018-12-31 price IOOLI 6 US
$ ./irr.py --currency USD --account Assets:Investments test.bc --from 2018-01-01 --to 2018-12-31
Traceback (most recent call last):
  File "./irr.py", line 256, in <module>
    r = xirr([(d, float(f)) for (d,f) in cashflows])
  File "./irr.py", line 66, in xirr
    return optimize.newton(lambda r: xnpv(r,cashflows),guess)
  File "/usr/lib/python3.6/site-packages/scipy/optimize/zeros.py", line 343, in newton
    raise RuntimeError(msg)
RuntimeError: Failed to converge after 50 iterations, value is 9.97188134707855e+22
redstreet commented 4 years ago

Case 2: Even though IOOLI's market price price matches the the price at conversion (usually never happens in real life), the results are still incorrect:

option "operating_currency" "USD"
plugin "beancount.plugins.auto_accounts"

2018-01-01 * "Buy"
   Assets:Investments 100 HOOLI {2 USD}
   Assets:Bank

2018-03-31 price IOOLI 4 USD

2018-04-01 * "Conversion"
   Assets:Investments -100 HOOLI {}
   Assets:Investments 50 IOOLI {4 USD}

2018-12-31 price HOOLI 3 USD
2018-12-31 price IOOLI 6 USD
$ ./irr.py --currency USD --account Assets:Investments test.bc --from 2018-01-01 --to 2018-12-31
332.10%
redstreet commented 4 years ago

Case 3: Typical case. Incorrect, but with a different IRR.

option "operating_currency" "USD"
plugin "beancount.plugins.auto_accounts"

2018-01-01 * "Buy"
   Assets:Investments 100 HOOLI {2 USD}
   Assets:Bank

2018-03-31 price IOOLI 7 USD

2018-04-01 * "Conversion"
   Assets:Investments -100 HOOLI {}
   Assets:Investments 50 IOOLI {4 USD}

2018-12-31 price HOOLI 3 USD
2018-12-31 price IOOLI 6 USD
$ ./irr.py --currency USD --account Assets:Investments test.bc --from 2018-01-01 --to 2018-12-31
27.49%

[(datetime.date(2018, 1, 1), Decimal('-100')),
 (datetime.date(2018, 1, 1), Decimal('100')),
 (datetime.date(2018, 4, 1), Decimal('250')),   # this is incorrect
 (datetime.date(2018, 12, 31), Decimal('-300'))]
redstreet commented 4 years ago

Case 4:

option "operating_currency" "USD"
plugin "beancount.plugins.auto_accounts"

2018-01-01 * "Buy"
   Assets:Investments 100 HOOLI {2 USD}
   Assets:Bank

2018-03-31 price HOOLI 3.50 USD
2018-03-31 price IOOLI 7 USD

2018-04-01 * "Conversion"
   Assets:Investments -100 HOOLI {}
   Assets:Investments 50 IOOLI {4 USD}

2018-12-31 price HOOLI 3 USD
2018-12-31 price IOOLI 6 USD
s ./irr.py --currency USD --account Assets:Investments test.bc --from 2018-01-01 --to 2018-12-31 --debug-inflows --debug-outflows --debug-cashflows
Traceback (most recent call last):
  File "/home/V/gnu/portfolio-returns/irr.py", line 256, in <module>
    r = xirr([(d, float(f)) for (d,f) in cashflows])
  File "/home/V/gnu/portfolio-returns/irr.py", line 66, in xirr
    return optimize.newton(lambda r: xnpv(r,cashflows),guess)
  File "/usr/lib/python3.6/site-packages/scipy/optimize/zeros.py", line 343, in newton
    raise RuntimeError(msg)
RuntimeError: Failed to converge after 50 iterations, value is 38034332453.872734
redstreet commented 4 years ago

Case 5: correct output, because of "beancount.plugins.implicit_prices"

option "operating_currency" "USD"
plugin "beancount.plugins.auto_accounts"
plugin "beancount.plugins.implicit_prices"

2018-01-01 * "Buy"
   Assets:Investments 100 HOOLI {2 USD}
   Assets:Bank

2018-04-01 * "Conversion"
   Assets:Investments -100 HOOLI {}
   Assets:Investments 50 IOOLI {4 USD}

2018-12-31 price HOOLI 3 USD
2018-12-31 price IOOLI 6 USD
irr.py --currency USD --account Assets:Investments test.bc --from 2018-01-01 --to 2018-12-31 --debug-inflows --debug-outflows --debug-cashflows
50.17%
[(datetime.date(2018, 1, 1), Decimal('200')),
 (datetime.date(2018, 12, 31), Decimal('-300'))]
>> [inflows]
{'Assets:Bank'}
<< [outflows]
set()
redstreet commented 4 years ago

Case 6: incorrect IRR because the conversion transaction cashflow is not zero (it should be zero).

option "operating_currency" "USD"
plugin "beancount.plugins.auto_accounts"
plugin "beancount.plugins.implicit_prices"

2018-01-01 * "Buy"
   Assets:Investments 100 HOOLI {2 USD}
   Assets:Bank

2018-03-31 price HOOLI 10 USD
2018-03-31 price IOOLI 20 USD

2018-04-01 * "Conversion"
   Assets:Investments -100 HOOLI {}
   Assets:Investments 50 IOOLI {4 USD}

2018-12-31 price HOOLI 3 USD
2018-12-31 price IOOLI 6 USD
$ ./irr.py --currency USD --account Assets:Investments test.bc --from 2018-01-01 --to 2018-12-31 --debug-inflows --debug-outflows --debug-cashflows
28162.27%
[(datetime.date(2018, 1, 1), Decimal('200')),
 (datetime.date(2018, 4, 1), Decimal('-800')),         # incorrect
 (datetime.date(2018, 12, 31), Decimal('-300'))]
>> [inflows]
{'Assets:Bank'}
<< [outflows]
set()
redstreet commented 4 years ago

I posted the test cases in the hope it is helpful, but I believe all these cases fall into two categories:

1) Unable to convert a posting into USD (likely because of what you said: beancount.core.convert.convert_position() fails)

2) The market price of HOOLI/IOOLI on 2018-04-01 should not matter, because it's a stock conversion. The cashflow on 2018-04-01 should be zero. However, it is non-zero likely because the value of one (or both) posting is incorrectly calculated. Note that beancount doesn't complain that the entry does not balance, meaning, it computes the transaction correctly. However, irr.py does not.

Thanks a ton for engaging on this! I'll dig into these bugs it if/when I can as well.

hoostus commented 4 years ago

Yeah, all of these are caused by convert_position() failing because the price map doesn't have enough information for it work.

While I investigate a real solution, I added some logging to detect when this happens and at least alert the user that convert_position() failed and the IRR is wrong :/

hoostus commented 4 years ago

Case 6: incorrect IRR because the conversion transaction cashflow is not zero (it should be zero).

This case is actually different than the others. The script sees you selling $1,000 of HOOLI and buying $200 of IOOLI, because the implicit_prices plugin is giving the 200 shares of IOOLI a price of $4/share.

If you remove the implicit_prices plugin and add an explicit price for the first HOOLI purchase like this:

2018-01-01 price HOOLI 2 USD

then it works as expected. I'll have to look into exactly what the implicit_prices plugin is doing that confuses things.

hoostus commented 4 years ago

I haven't forgotten about this...I was just lazy over the holidays :)

First the easy part:

Having thought about it more, I think Case #6 is simply not a valid case. If you want to migrate a position like that then you need to add a date as well:

Assets:Investments -100 HOOLI {}
Assets:Investments 50 IOOLI {2018-01-01, 4 USD}

This will keep the age of the position correct (for long-term, short-term tax tracking) and also won't confuse the implicit prices plugin.

So I think we can strike Case #6 from the problems. But that still leaves the others.....

redstreet commented 4 years ago

Agreed about case 6. Including the original date when specifying the lot is what makes sense.

Beancount does accept case 6 without a date, but whether or not this plugin should at least complain rather than produce incorrect results is more of an academic one, than a practical one :).

hoostus commented 4 years ago

Having looked at the beancount code for how it determines if a transaction balances...it puts everything together into an Inventory object which then magically calculates residuals....So what is it doing differently from this script?

It is using beancount.core.convert.get_weight() instead of beancount.core.convert.convert_position().

convert_position() has the bonus of doing currency conversions for us, which would be nice to be able to keep. I think I'll have to post on the mailing list and see if anyone has any better ideas on how to approach this.

ankurdave commented 2 years ago

Using beancount.core.convert.get_weight() seems like the right solution to me. We should only require price information to determine the starting and ending balances, not to derive the cashflows in between.

If the weight's currency differs from the user's desired currency, we can still use beancount.core.convert.convert_amount() to do the currency conversion at the appropriate price.

Using get_weight() also simplifies the categorization of "internal" cashflows. Dividends, interest, fees, capital gains, and capital gains distributions can all be treated as internal. For example, the following transaction represents an outflow of 2500 USD, which is what we get when treating Assets:Brokerage and Income:CapitalGains as internal:

2017-12-01 * "Sell 2,000 shares"
    Assets:Brokerage     -1,000 ABC {1.00 USD}
    Assets:Brokerage     -1,000 ABC {2.00 USD}
    Assets:Cash           2,500 USD
    Income:CapitalGains     500 USD

I implemented the above and added tests derived from the open issues: https://github.com/ankurdave/portfolio-returns. Happy to submit a PR with these changes.

redstreet commented 2 years ago

Related: beangrow, which I now use, solves a lot of the issues around returns calculations quite comprehensively IMHO. It takes a two-pass approach where the first pass outputs all the time points at which a price entry is required to make accurate calculations.

ankurdave commented 2 years ago

I did give beangrow a try after seeing your suggestion and review on the beancount mailing list. I was put off by what seemed like excessive complexity for my use case (calculate the total IRR for a group of investments), which portfolio-returns handles elegantly aside from the price/cost confusion. But I'm starting to see the need for more complexity if I wanted to break down the IRRs for each individual investment and due to the various sources (dividends, capital appreciation, fees). I'll give it another look.

hoostus commented 2 years ago

Sure, if you submit a PR I'll apply it.

Thanks!

------- Original Message ------- On Thursday, August 25th, 2022 at 12:03 AM, Ankur Dave @.***> wrote:

Using beancount.core.convert.get_weight() seems like the right solution to me. We should only require price information to determine the starting and ending balances, not to derive the cashflows in between.

If the weight's currency differs from the user's desired currency, we can still use beancount.core.convert.convert_amount() to do the currency conversion at the appropriate price.

Using get_weight() also simplifies the categorization of "internal" cashflows. Dividends, interest, fees, capital gains, and capital gains distributions can all be treated as internal. For example, the following transaction represents an outflow of 2500 USD, which is what we get when treating Assets:Brokerage and Income:CapitalGains as internal:

2017-12-01 * "Sell 2,000 shares" Assets:Brokerage -1,000 ABC {1.00 USD} Assets:Brokerage -1,000 ABC {2.00 USD} Assets:Cash 2,500 USD Income:CapitalGains 500 USD

I implemented the above and added tests derived from the open issues: https://github.com/ankurdave/portfolio-returns. Happy to submit a PR with these changes.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>