Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--historical uses period-end valuations of prices, while --cumulative doesn't #2205

Open
the-solipsist opened this issue Jun 17, 2024 · 17 comments
Labels
A-WISH Some kind of improvement request, hare-brained proposal, or plea. docs Documentation-related. valuation

Comments

@the-solipsist
Copy link
Collaborator

--historical and --cumulative provided the same output until v1.20 if the periods were the same.

Since v1.21, they no longer do. They diverge if there are multiple commodities with prices that change over the periods.

I'm not sure if this should be counted as a regression or not.

Example

sol@spica /tmp> hledger --version
hledger 1.33.1, linux-x86_64

sol@spica /tmp> cat temp.j
2020-01-01 Transaction
  Assets:Cash:INR     INR 10000.00
  Assets:Cash:USD     USD 2000.00
  Equity:Opening

P 2020-12-31 USD INR 40.00
P 2021-12-31 USD INR 50.00
P 2022-12-31 USD INR 60.00

sol@spica /tmp> hledger -f temp.j bal --yearly -XINR --cumulative
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

                 ||    2020-12-31     2021-12-31     2022-12-31 
=================++=============================================
 Assets:Cash:INR ||  INR 10000.00   INR 10000.00   INR 10000.00 
 Assets:Cash:USD ||  INR 80000.00   INR 80000.00   INR 80000.00 
 Equity:Opening  || INR -90000.00  INR -90000.00  INR -90000.00 
-----------------++---------------------------------------------
                 ||             0              0              0 

sol@spica /tmp> hledger -f temp.j bal --yearly -XINR --historical
Ending balances (historical) in 2020-01-01..2022-12-31, valued at period ends:

                 ||    2020-12-31      2021-12-31      2022-12-31 
=================++===============================================
 Assets:Cash:INR ||  INR 10000.00    INR 10000.00    INR 10000.00 
 Assets:Cash:USD ||  INR 80000.00   INR 100000.00   INR 120000.00 
 Equity:Opening  || INR -90000.00  INR -110000.00  INR -130000.00 
-----------------++-----------------------------------------------
                 ||             0               0               0 
@the-solipsist the-solipsist added severity2 Minor to moderate usability/doc bug, reasonably easy to avoid or tolerate. impact4 Affects more than a few users. labels Jun 17, 2024
@the-solipsist the-solipsist changed the title --historical uses period-end valuation of prices, while --cumulative does not --historical uses period-end valuations of prices, while --cumulative doesn't Jun 17, 2024
@simonmichael
Copy link
Owner

I will definitely check it out, thanks!

@simonmichael simonmichael added valuation impact3 Affects just a few users. and removed impact4 Affects more than a few users. labels Jul 20, 2024
@simonmichael
Copy link
Owner

simonmichael commented Jul 20, 2024

I found this again. Here's another repro:

2020-01-01
  (Assets)     USD 1

P 2020-12-31 USD INR 40
P 2021-12-31 USD INR 50
P 2022-12-31 USD INR 60

1.20 values each cumulative balance with that month's price, which seems correct as month-end price is supposed to be the default for a multi-period report:

~/src/hledger$ hledger-1.20.3 -f 2024-07-20/2205.j bal -Y -XINR --cumulative -p 2020-2023 -EN
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31 
========++====================================
 Assets ||     INR 40      INR 50      INR 60 

1.21 and later values all cumulative balances with the first month's price, which seems wrong, and I guess a regression ?:

~/src/hledger$ hledger-1.21 -f 2024-07-20/2205.j bal -Y -XINR --cumulative -p 2020-2023 -EN
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31 
========++====================================
 Assets ||     INR 40      INR 40      INR 40 

@simonmichael simonmichael added A-BUG Something wrong, confusing or sub-standard in the software, docs, or user experience. regression A backwards step, indicating a weakness in our QA. We don't like these. labels Jul 20, 2024
@kkoniuszy
Copy link

I bisected this. The first commit that changes the output for provided repros is 7f2536a.

@simonmichael
Copy link
Owner

Thank you! I was looking at this but had to put it aside for a while.

I'm not certain why valuation was dropped/changed for --cumulative , it may have been just a misunderstanding or
oversight.

@Xitian9
Copy link
Collaborator

Xitian9 commented Aug 2, 2024

This was what came out of a very extensive discussion here #1353.

Essentially, when looking at a cumulative report with valuation, there are two different things you might want presented:

  1. Valuation of the change: In each period, you take a look at what the change in an account was, and then apply the valuation to the change.
  2. Change in the valuation: In each period you apply the valuation to the end balance, and then look at the differences in those valuations.

Each is useful in certain circumstances, and we decided that "valuation of the change" was the obvious default for change reports, "change in the valuation" was the obvious default for historical reports, and that cumulative reports were an odd duck where you would sometimes want one and sometimes want the other. If you want the "change in valuation behaviour" instead of the "valuation of the change" behaviour, you can supply the --valuechange flag.

@the-solipsist
Copy link
Collaborator Author

I can confirm that. The following two outputs are identical.

sol@spica /tmp> hledger -f temp.j bal --yearly -XINR --historical
Ending balances (historical) in 2020-01-01..2022-12-31, valued at period ends:

                 ||    2020-12-31      2021-12-31      2022-12-31 
=================++===============================================
 Assets:Cash:INR ||  INR 10000.00    INR 10000.00    INR 10000.00 
 Assets:Cash:USD ||  INR 80000.00   INR 100000.00   INR 120000.00 
 Equity:Opening  || INR -90000.00  INR -110000.00  INR -130000.00 
-----------------++-----------------------------------------------
                 ||             0               0               0 
sol@spica /tmp> hledger -f temp.j bal --yearly -XINR --cumulative --valuechange
Cumulative period-end value changes in 2020-01-01..2022-12-31:

                 ||    2020-12-31      2021-12-31      2022-12-31 
=================++===============================================
 Assets:Cash:INR ||  INR 10000.00    INR 10000.00    INR 10000.00 
 Assets:Cash:USD ||  INR 80000.00   INR 100000.00   INR 120000.00 
 Equity:Opening  || INR -90000.00  INR -110000.00  INR -130000.00 
-----------------++-----------------------------------------------
                 ||             0               0               0 

Perhaps what might be needed is for the docs to be clarified. The explanation @Xitian9 has provided can be used, perhaps?

We can close this as a non-regression / non-bug.

@the-solipsist
Copy link
Collaborator Author

I bisected this. The first commit that changes the output for provided repros is 7f2536a.

@kkoniuszy, this is off-topic, but could you please share the git commands you ran in order to figure out the exact commit that introduced this change? Thanks. I don't know how to use git bisect and a practical example such as this would be valuable.

@kkoniuszy
Copy link

@the-solipsist Sure.

First, start the bisect, then mark 1.21 as bad and 1.20 as good.

~/dev/github/hledger master
❯ git bisect start
status: waiting for both good and bad commits


~/dev/github/hledger master
❯ git bisect bad 1.21
status: waiting for good commit(s), bad commit known


~/dev/github/hledger master
❯ git bisect good 1.20
Bisecting: 222 revisions left to test after this (roughly 8 steps)
[84bf42a9fc038120065cee77b5a075c6013d9ae1] ;bin: linux/pr CI uses ghc 8.2, make functest uses default ghc (#1453)

Then, on every bisect iteration, try to reproduce the error and run either git bisect good or git bisect bad depending on the output. This shouldn't take long.

~/dev/github/hledger tags/hledger-1.21~128
❯ stack run -j32 --resolver lts-18.28 -- hledger -f /tmp/repro.j bal -Y -XINR --cumulative -p 2020-2023 -EN; git restore .
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31 
========++====================================
 Assets ||     INR 40      INR 50      INR 60 


~/dev/github/hledger tags/hledger-1.21~128
❯ git bisect good
Bisecting: 111 revisions left to test after this (roughly 7 steps)
[c2da8ac2d49d5e60b4eb76205c64983efafce4b7] Fix extension completion


~/dev/github/hledger tags/hledger-1.21~25^2~67
❯ stack run -j32 --resolver lts-18.28 -- hledger -f /tmp/repro.j bal -Y -XINR --cumulative -p 2020-2023 -EN; git restore .
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31 
========++====================================
 Assets ||     INR 40      INR 40      INR 40 


~/dev/github/hledger tags/hledger-1.21~25^2~67
❯ git bisect bad
Bisecting: 55 revisions left to test after this (roughly 6 steps)
[61013689546433628f175883a7b34ec9a91b5f0c] print: always show all decimal places  (#931)


~/dev/github/hledger tags/hledger-1.21~80
❯ stack run -j32 --resolver lts-18.28 -- hledger -f /tmp/repro.j bal -Y -XINR --cumulative -p 2020-2023 -EN; git restore .
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31 
========++====================================
 Assets ||     INR 40      INR 50      INR 60 


~/dev/github/hledger tags/hledger-1.21~80
❯ git bisect good
Bisecting: 27 revisions left to test after this (roughly 5 steps)
[150cf3f86223ed42824292125a99388ddb0b68c9] roi: roi-unrealised.example now works based on price directives


~/dev/github/hledger tags/hledger-1.21~52
❯ stack run -j32 --resolver lts-18.28 -- hledger -f /tmp/repro.j bal -Y -XINR --cumulative -p 2020-2023 -EN; git restore .
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31 
========++====================================
 Assets ||     INR 40      INR 50      INR 60 


~/dev/github/hledger tags/hledger-1.21~52
❯ git bisect good
Bisecting: 13 revisions left to test after this (roughly 4 steps)
[72b737a42f1d719d7888aac4ddf62fe4e0c5a2a5] Fix #1404, and more...


~/dev/github/hledger tags/hledger-1.21~25^2~81
❯ stack run -j32 --resolver lts-18.28 -- hledger -f /tmp/repro.j bal -Y -XINR --cumulative -p 2020-2023 -EN; git restore .
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31 
========++====================================
 Assets ||     INR 40      INR 40      INR 40 


~/dev/github/hledger tags/hledger-1.21~25^2~81
❯ git bisect bad
Bisecting: 7 revisions left to test after this (roughly 3 steps)
[f0655d1c7f2269c3b9fdceb79c6b79ef7cd6cf41] lib: (amount|mixedAmount)(Looks|Is)Zero functions now check whether both the quantity and the cost are zero. This is usually what you want, but if you do only want to check whether the quantity is zero, you can run mixedAmountStripPrices (or similar) before this.


~/dev/github/hledger tags/hledger-1.21~50^2
❯ stack run -j32 --resolver lts-18.28 -- hledger -f /tmp/repro.j bal -Y -XINR --cumulative -p 2020-2023 -EN; git restore .
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31 
========++====================================
 Assets ||     INR 40      INR 50      INR 60 


~/dev/github/hledger tags/hledger-1.21~50^2
❯ git bisect good
Bisecting: 3 revisions left to test after this (roughly 2 steps)
[351648e4fac30593cbf612a7ab7cd0aa35286694] lib,cli: Add --periodic option to indicate PeriodChange accumulation (renamed from --change).


~/dev/github/hledger tags/hledger-1.21~48
❯ stack run -j32 --resolver lts-18.28 -- hledger -f /tmp/repro.j bal -Y -XINR --cumulative -p 2020-2023 -EN; git restore .
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31 
========++====================================
 Assets ||     INR 40      INR 50      INR 60 


~/dev/github/hledger tags/hledger-1.21~48
❯ git bisect good
Bisecting: 1 revision left to test after this (roughly 1 step)
[f63c38a7e2c6f94e2d5c2bd7b76d9c63cbaf0081] bal: docs rewritten, and updated for new flags


~/dev/github/hledger tags/hledger-1.21~46
❯ stack run -j32 --resolver lts-18.28 -- hledger -f /tmp/repro.j bal -Y -XINR --cumulative -p 2020-2023 -EN; git restore .
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31 
========++====================================
 Assets ||     INR 40      INR 40      INR 40 


~/dev/github/hledger tags/hledger-1.21~46
❯ git bisect bad
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[7f2536a2a79c3aae58d1c98dd3c06cc3b5166bf1] lib,cli: Add --valuechange report type for calculating change of value of accounts, restore --value=end behaviour to that of hledger-1.19.


~/dev/github/hledger tags/hledger-1.21~47
❯ stack run -j32 --resolver lts-18.28 -- hledger -f /tmp/repro.j bal -Y -XINR --cumulative -p 2020-2023 -EN; git restore .
Ending balances (cumulative) in 2020-01-01..2022-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31 
========++====================================
 Assets ||     INR 40      INR 40      INR 40 

This is the last revision to test, so git bisect bad will finally end the bisect and show the commit.

~/dev/github/hledger tags/hledger-1.21~47
❯ git bisect bad
7f2536a2a79c3aae58d1c98dd3c06cc3b5166bf1 is the first bad commit
commit 7f2536a2a79c3aae58d1c98dd3c06cc3b5166bf1 (HEAD)
Author: Stephen Morgan <[email protected]>
Date:   Mon Feb 8 15:31:17 2021 +1100

    lib,cli: Add --valuechange report type for calculating change of value
    of accounts, restore --value=end behaviour to that of hledger-1.19.

 hledger-lib/Hledger/Reports/MultiBalanceReport.hs |  31 ++++++++++++++++++-------------
 hledger-lib/Hledger/Reports/PostingsReport.hs     |   6 +++---
 hledger-lib/Hledger/Reports/ReportOptions.hs      |  44 +++++++++++++++++++++++++++++++++-----------
 hledger/Hledger/Cli/Commands/Balance.hs           |  46 ++++++++++++++++++++++++++--------------------
 hledger/Hledger/Cli/CompoundBalanceCommand.hs     |  17 +++++++++++++----
 hledger/test/journal/valuation.test               | 102 ++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------
 hledger/test/journal/valuechange.test             |  55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 192 insertions(+), 109 deletions(-)
 create mode 100644 hledger/test/journal/valuechange.test

You can now run git bisect reset to get your local repo back to the state before bisecting.

~/dev/github/hledger tags/hledger-1.21~47
❯ git bisect reset
Previous HEAD position was 7f2536a2a lib,cli: Add --valuechange report type for calculating change of value of accounts, restore --value=end behaviour to that of hledger-1.19.
Switched to branch 'master'
Your branch is up to date with 'origin/master'.

@kkoniuszy
Copy link

kkoniuszy commented Aug 4, 2024

@Xitian9 @simonmichael I wonder if the report header isn't kind of misleading.

It says valued at period ends, but this isn't really true – starting with 1.21, --cumulative --value=end isn't valued at the end of each period anymore. It's valued at the end of the first one, so for this test case it acts more like --cumulative --value=2020-12-31 would in version 1.20. Something like valued at the end of the first period (2020-12-31) might make more sense.

@Xitian9
Copy link
Collaborator

Xitian9 commented Aug 4, 2024

No, it's not valued at the end of the first period: each change is valued in the period in which it occurs. The difference is that it is not re-evaluated later on. You can see this from an extension of @simonmichael's example above.

2020-01-01
  (Assets)     USD 1

2022-01-01
  (Assets)     USD 1

P 2020-12-31 USD INR 40
P 2021-12-31 USD INR 50
P 2022-12-31 USD INR 60
P 2023-12-31 USD INR 70
$ hledger -f test.j bal -Y -XINR --cumulative -p 2020-2024 -EN                                                                                                                                                                                                                                                                           2024-08-04 21:30
Ending balances (cumulative) in 2020-01-01..2023-12-31, valued at period ends:

        || 2020-12-31  2021-12-31  2022-12-31  2023-12-31
========++================================================
 Assets ||     INR 40      INR 40     INR 100     INR 100

You can see that the first transaction is valued at the INR 40 that it would be on 2020-12-31, while the second is valued at the INR 60 that it would be on 2022-12-31.

The way to think about the behaviour is that it operates as a cumulative version of the change report, i.e. what you would get if you just added up all the entries of the change report:

$ hledger -f test.j bal -Y -XINR -p 2020-2024 -EN                                                                                                                                                                                                                                                                                        2024-08-04 21:31
Balance changes in 2020-01-01..2023-12-31, valued at period ends:

        ||   2020  2021    2022  2023
========++============================
 Assets || INR 40     0  INR 60     0

@Xitian9
Copy link
Collaborator

Xitian9 commented Aug 4, 2024

It may help to think of the two different behaviours in terms of where they would be useful.

The default behaviour of the change and cumulative reports is what you should think about for things like expenses or revenues. If 10 year ago I bought a cabbage for 1 USD and consumed it, after which the value of the INR against the USD collapsed so that that cabbage would now cost 60 INR instead of 40, I wouldn't expect to see that appear in my change or cumulative reports.

On the other hand, if I bought 1 USD and stored it (as an asset, or maybe a liability or equity), after which the value of the INR collapsed, then I would be able to sell my USD for more INR than previously. This is what --valuechange is used for. If you run a change report with --valuechange then you can see balances in periods in which there were no transactions, due solely to the fluctuation in value:

$ hledger -f test.j bal -Y -XINR -p 2020-2024 -EN --valuechange                                                                                                                                                                                                                                                                          2024-08-04 21:42
Period-end value changes in 2020-01-01..2023-12-31:

        ||   2020    2021    2022    2023
========++================================
 Assets || INR 40  INR 10  INR 70  INR 20

Different use cases where each would be needed. Generally the default behaviour is more useful for expenses and revenues, the value-change behaviour for assets, liabilities, and equity.

@kkoniuszy
Copy link

Thanks for the explanation, it's much clearer now.

@the-solipsist
Copy link
Collaborator Author

It may help to think of the two different behaviours in terms of where they would be useful.

The default behaviour of the change and cumulative reports is what you should think about for things like expenses or revenues. If 10 year ago I bought a cabbage for 1 USD and consumed it, after which the value of the INR against the USD collapsed so that that cabbage would now cost 60 INR instead of 40, I wouldn't expect to see that appear in my change or cumulative reports.

On the other hand, if I bought 1 USD and stored it (as an asset, or maybe a liability or equity), after which the value of the INR collapsed, then I would be able to sell my USD for more INR than previously. This is what --valuechange is used for. If you run a change report with --valuechange then you can see balances in periods in which there were no transactions, due solely to the fluctuation in value.

That's a very good point, I think it is also worth explaining in the manual that --valuechange is more useful with periodic reports for Assets, Liabilities, and Equity, while bare -P / -M / -Y, etc., balance reports are more valuable with Income and Expenses.

@the-solipsist
Copy link
Collaborator Author

It would be worth noting that:

  1. historical and --cumulative --valuechange are identical if the period includes the first relevant transaction.
  2. historical and --cumulative are only identical if (a) the period includes the first relevant transaction AND (b) it is only a single currency that's used / there are no changes in market prices if multiple currencies are used.

Also, I'll note that the following three provide identical results for a journal that starts from 2017-01-01:

hledger bse -p "yearly from 2016-04" -X₹ --depth=1
hledger bse -p "yearly from 2016-04" -X₹ --depth=1 --historical
hledger bse -p "yearly from 2016-04" -X₹ --depth=1 --cumulative --valuechange

while this provides a different result:
hledger bse -p "yearly from 2016-04" -X₹ --depth=1 --cumulative

So the default for -P with bse (i.e., --historical / --cumulative --valuechange) makes sense.

Similarly, these two are identical
hledger is -p "yearly from 2016-04" -X₹ --depth=1 not:tag:clopen
hledger is -p "yearly from 2016-04" -X₹ --depth=1 not:tag:clopen --change

The default for -P with is (i.e., --change) kind of makes sense

However, the best for incomestatement (as @Xitian9's has explained) would be --change --value=then --infer-market-prices, but that is computationally expensive.

@simonmichael simonmichael added docs Documentation-related. and removed regression A backwards step, indicating a weakness in our QA. We don't like these. labels Aug 4, 2024
@simonmichael
Copy link
Owner

Thanks @Xitian9, & all, for the insightful comments!

I have converted this to a doc bug, let's try to include these explanations in the manual insofar as they can fit, or perhaps in a separate website doc. I'll work on it if no-one beats me to it.

@simonmichael
Copy link
Owner

simonmichael commented Dec 4, 2024

I reviewed this discussion. Perhaps we need a more extensive doc/page/faq section/notes that explores this topic (valuation, periodic reports..) in detail. From this we could probably distill more succinct tips and links to include in the manual.

@simonmichael
Copy link
Owner

Step 1 would be to survey what we have, I guess.

@simonmichael simonmichael added A-WISH Some kind of improvement request, hare-brained proposal, or plea. and removed A-BUG Something wrong, confusing or sub-standard in the software, docs, or user experience. severity2 Minor to moderate usability/doc bug, reasonably easy to avoid or tolerate. labels Dec 4, 2024
@simonmichael simonmichael removed the impact3 Affects just a few users. label Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-WISH Some kind of improvement request, hare-brained proposal, or plea. docs Documentation-related. valuation
Projects
None yet
Development

No branches or pull requests

4 participants