simonmichael / hledger

Robust, fast, intuitive plain text accounting tool with CLI, TUI and web interfaces.
https://hledger.org
GNU General Public License v3.0
2.97k stars 317 forks source link

roi valuation refuses to unify accounts, gives incorrect recommendation #2190

Closed aragaer closed 6 months ago

aragaer commented 6 months ago

Given the following journal file:

P 2023-01-01 B 20A
P 2023-01-01 C 1B

2023-01-01
 investment  1C @@ 20A
 investment  4B @@ 80A
 assets

P 2023-12-31 C 2B

If I want to get ROI "in units of B", I'm expecting to see something like this:

$ hledger -f /tmp/test.journal roi --inv investment --pnl income --value='end,B' -e2024
+---++------------+------------++---------------+----------+-------------+-----++--------++------------+----------+
|   ||      Begin |        End || Value (begin) | Cashflow | Value (end) | PnL ||    IRR || TWR/period | TWR/year |
+===++============+============++===============+==========+=============+=====++========++============+==========+
| 1 || 2023-01-01 | 2023-12-31 ||             0 |       5B |          6B |  1B || 20.00% ||     20.00% |   20.00% |
+---++------------+------------++---------------+----------+-------------+-----++--------++------------+----------+

But instead I get an error Error: Amounts could not be converted to a single cost basis: ["2B","4B @@ 80A"] with a recommendation to add --cost.

Adding --cost helps remove the error message, but gives an incorrect result:

$ hledger -f /tmp/test.journal roi --inv investment --pnl income --value='end,B' -e2024 --cost
+---++------------+------------++---------------+----------+-------------+-----++-------++------------+----------+
|   ||      Begin |        End || Value (begin) | Cashflow | Value (end) | PnL ||   IRR || TWR/period | TWR/year |
+===++============+============++===============+==========+=============+=====++=======++============+==========+
| 1 || 2023-01-01 | 2023-12-31 ||             0 |       5B |          5B |   0 || 0.00% ||      0.00% |    0.00% |
+---++------------+------------++---------------+----------+-------------+-----++-------++------------+----------+

How should I change my query (or perhaps my journal) to see that 20% IRR?

aragaer commented 6 months ago

It does work as expected if I rewrite the transaction like this:

2023-01-01
 investment  4B
 investment  1C
 assets  -80A @@ 4B
 assets  -20A @@ 1C

Which seems to be completely equivalent but is somehow different.

simonmichael commented 6 months ago

I don't know roi well, but it is true in general that the order of postings is significant when using @ cost notation. It is mentioned briefly at

simonmichael commented 6 months ago

Perhaps a patch to roi's help is needed ?

aragaer commented 6 months ago

It looks like cashflow records are processed so that the they are all converted to a single commodity. However initial and final values are not. At the moment I can't figure a simple way to fix that.

The issue appears if during computation of either initial or final value we encounter a posting that uses the required commodity, but also has a price in some other commodity. In the example above: 1C @@ 20A is converted into 1B, 1C is also converted to 1B, 4B is just 4B but 4B @@ 20A stays as a separate commodity and is not converted to B and thus is a "separate" commodity.

aragaer commented 6 months ago

I think that in this specific case it's a bug in roi implementation, not documentation issue. It should not try and unMix ["B", "B @@ 20A"] in general case (that is roi test 9), but with --value=then,B it should see those two as equal. I think the problem is that --value is not applied to valueBefore and valueAfter while it is applied to cashFlow.

Here's a different, somewhat simpler example with the same issue:

2023-01-01 B 1A

2023-01-01
 investment  1B
 investment  1B @@ 1A
 assets
adept commented 6 months ago

That's a bug allright (but not where you say it is - in fact, both valueBefore and valueAfter are properly valued at the moment). The fix is forthcoming.

simonmichael commented 6 months ago

Merged, thanks!