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

--infer-market-prices infers negative prices #1870

Closed
simonmichael opened this issue Jun 6, 2022 · 21 comments
Closed

--infer-market-prices infers negative prices #1870

simonmichael opened this issue Jun 6, 2022 · 21 comments
Labels
A-BUG Something wrong, confusing or sub-standard in the software, docs, or user experience. docs Documentation-related. valuation

Comments

@simonmichael
Copy link
Owner

simonmichael commented Jun 6, 2022

[Related: #1813, #1816]

With this entry:

2022-01-01
    a        A -1
    b        B 1 @@ A 1

hledger 1.25 and 1.26 infer the same market price:

$ hledger-1.25 prices --infer-market-prices
P 2022-01-01 B A 1.0
$ hledger-1.26 prices --infer-market-prices
P 2022-01-01 B A 1.0

and produce the same valuation:

$ hledger-1.25 print --infer-market-prices -V
2022-01-01
    a            A -1
    b             A 1

$ hledger-1.26 print --infer-market-prices -V
2022-01-01
    a            A -1
    b             A 1

But with this entry, which is odd but currently considered legal and balanced, since the second posting's negatives cancel out:

2022-01-01
    a        A -1
    b        B -1 @@ A -1

hledger 1.25 inferred the market price correctly but produced an incorrect valuation (note both amounts are negative):

$ hledger-1.25 prices --infer-market-prices
P 2022-01-01 B A 1.0
$ hledger-1.25 print --infer-market-prices -V
2022-01-01
    a            A -1
    b            A -1

hledger 1.26 fixed the valuation, but now incorrectly infers a negative market price:

$ hledger-1.26 prices --infer-market-prices
P 2022-01-01 B A -1.0
$ hledger-1.26 print --infer-market-prices -V
2022-01-01
    a            A -1
    b             A 1

I had several of those odd double negative entries (generated by csv rules, unknowingly), and this caused hledger 1.26 to report an incorrect balance in one of my reports. A minimal example:

$ hledger-1.25 bal b -N -X A --infer-market-prices
                A -1  b
$ hledger-1.26 bal b -N -X A --infer-market-prices
                 A 1  b
@simonmichael simonmichael added A-BUG Something wrong, confusing or sub-standard in the software, docs, or user experience. valuation regression A backwards step, indicating a weakness in our QA. We don't like these. labels Jun 6, 2022
@simonmichael simonmichael changed the title --infer-market-prices infers negative prices in a new way --infer-market-prices infers negative prices Jun 6, 2022
@Xitian9
Copy link
Collaborator

Xitian9 commented Jun 7, 2022

Ugh, the total price for negative values is extremely counter-intuitive. But looking at this, I don't see why this isn't a negative price. If you think the correct valuation of B -1 @@ A -1 is A 1, then we have that B -1 is equivalent to A 1, which is a negative price.

But regardless of how this particular issue gets resolved, we really need to make this more intuitive. I was the one who wrote this and I can't get my head straight about what the correct behaviour should be.

@Xitian9
Copy link
Collaborator

Xitian9 commented Jun 7, 2022

Here is a proposed test suite. Here are the properties I believe we should satisfy.

  1. All transactions are balanced.
  2. Transactions on the same day are equivalent (they should be the same thing written in unit or total prices).
  3. --cost should be equivalent to --infer-market-prices --value=then.
2022-01-01 Positive Unit prices
    a        A 1
    b        B -1 @ A 1

2022-01-01 Positive Total prices
    a        A 1
    b        B -1 @@ A 1

2022-01-02 Negative unit prices
    a        A 1
    b        B 1 @ A -1

2022-01-02 Negative total prices
    a        A 1
    b        B 1 @@ A -1

2022-01-03 Double Negative unit prices
    a        A -1
    b        B -1 @ A -1

2022-01-03 Double Negative total prices
    a        A -1
    b        B -1 @@ A -1

Based on my testing for hledger-1.26, all these hold except (3) for the transactions on 2022-01-01. These pass all these tests, provided you're sure to use --value=then to get the valuation on the date of the transaction. Let me know if you agree.

@simonmichael
Copy link
Owner Author

simonmichael commented Jun 9, 2022

Thanks for the examples and notes. More: here are the market prices inferred from the above by 1.25 and 1.26:

; $ hledger-1.25 prices --infer-market-prices
P 2022-01-01 B A 1
P 2022-01-01 B A -1.0
P 2022-01-02 B A -1
P 2022-01-02 B A -1.0
P 2022-01-03 B A -1
P 2022-01-03 B A 1.0
; $ hledger-1.26 prices --infer-market-prices
P 2022-01-01 B A 1
P 2022-01-01 B A 1.0
P 2022-01-02 B A -1
P 2022-01-02 B A -1.0
P 2022-01-03 B A -1
P 2022-01-03 B A -1.0

Questions: should we allow or disallow negative transaction prices and negative market prices, what do they mean, and if allowed how exactly should they work ?

I have not used negative prices or seen them used intentionally.

Our docs do not mention them (and Ledger's and Beancount's probably don't either). Here are some related places:

  1. https://hledger.org/dev/hledger.html#transaction-prices
  2. https://hledger.org/dev/hledger.html#declaring-market-prices
  3. https://hledger.org/dev/hledger.html#market-prices

1 -> item 3 describes entries where conversion price is left implicit. hledger always rejects such an entry if both amounts have the same sign, so that's one place where we enforce that the inferred price is positive.

One way to simplify would be to disallow negative transaction prices and negative market prices. Did we discuss and reject this in the past ?

@simonmichael
Copy link
Owner Author

simonmichael commented Jun 9, 2022

Ledger currently disallows negative transaction prices (costs), but allows negative market prices. Beancount does the same.

@simonmichael
Copy link
Owner Author

--cost should be equivalent to --infer-market-prices --value=then

I think that won't always be true since a P directive on the same day can override an inferred market price.

@Xitian9
Copy link
Collaborator

Xitian9 commented Jun 9, 2022

--cost should be equivalent to --infer-market-prices --value=then

I think that won't always be true since a P directive on the same day can override an inferred market price.

True, I was referring to the more limited case in this example, where there were no price directives. To be careful, we should also expand those transactions so they're all on different days.

@Xitian9
Copy link
Collaborator

Xitian9 commented Jun 9, 2022

One way to simplify would be to disallow negative transaction prices and negative market prices. Did we discuss and reject this in the past ?

Internally we have to allow negative transaction prices, as that might be the result of summing two positive total prices:
$50 @@ €40 + (-$60 @@ €30) == -$10 @@ -€10. In words, if I buy $50 at a cost of €40, and then sell $60 and receive €30 for them, the overall result is that I've lost $10 and €10; that's a negative price.

We could disallow negative prices in journals, but I can see this running into problems if any internal negative prices leak out into a journal, for example with the close or print commands.

@simonmichael
Copy link
Owner Author

Here's real world https://en.wikipedia.org/wiki/Negative_pricing, also.

@simonmichael
Copy link
Owner Author

simonmichael commented Jun 10, 2022

Aside: above, prices shows the prices inferred from @@ with a .0, it would be nice to hide that / show all prices with consistent format.

@simonmichael
Copy link
Owner Author

After discussion and re-reading everything, I'm starting to get used to the idea that 1.26 is handling my and your examples correctly and all that may be needed is more documentation about our support for negative prices and how that works with @ and @@.

@Xitian9
Copy link
Collaborator

Xitian9 commented Jun 10, 2022

After discussion and re-reading everything, I'm starting to get used to the idea that 1.26 is handling my and your examples correctly and all that may be needed is more documentation about our support for negative prices and how that works with @ and @@.

Agreed.

@simonmichael
Copy link
Owner Author

simonmichael commented Aug 17, 2022

Hmm, returning to this two months later.. I wonder what should docs be saying about negative prices. "We allow both transaction and market prices to be negative, and here's a few examples" ?

@simonmichael simonmichael added the docs Documentation-related. label Aug 17, 2022
@tanelihuuskonen
Copy link

tanelihuuskonen commented Oct 24, 2022

I'm a mathematician with some understanding of the basics of accounting. It seems fairly obvious to me that most of the counterintuitiveness and difficulties concerning negative unit prices are just symptoms of an underlying design decision, inherited from Ledger, that just doesn't really mix with negative prices. The root of the problem is that the total price of a negative quantity of a normal (that is, positively priced) commodity is negative, but for some obscure reason, the program pretends otherwise, flipping the sign when reading and writing such entries. If you stop that kind of fibbing, the entries for negatively-priced commodities will look far less confusing, at least for me. The following examples would then balance without a dummy equity account and show that 1 POS and 1 NEG are consistently worth 2.00 FOO and -2.00 FOO, respectively (again, the transactions occurring on the same day are meant to be identical, only written differently):

commodity 1.00 FOO
commodity 1.00 POS
commodity 1.00 NEG

2022-01-01 Buying POS, unit price
assets:stock 3.00 POS @ 2.00 FOO
assets:bank -6.00 FOO

2022-01-01 Buying POS, total price
assets:stock 3.00 POS @@ 6.00 FOO
assets:bank -6.00 FOO

2022-01-02 Selling POS, unit price
assets:stock -3.00 POS @ 2.00 FOO
assets:bank 6.00 FOO

2022-01-02 Selling POS, total price
assets:stock -3.00 POS @@ -6.00 FOO
assets:bank 6.00 FOO
equity:error

2022-02-01 Buying NEG (getting paid to receive it), unit price
assets:stock 3.00 NEG @ -2.00 FOO
assets:bank 6.00 FOO

2022-02-01 Buying NEG, total price
assets:stock 3.00 NEG @@ -6.00 FOO
assets:bank 6.00 FOO

2022-02-02 Selling NEG (paying someone to take it away), unit price
assets:stock -3.00 NEG @ -2.00 FOO
assets:bank -6.00 FOO

2022-02-02 Selling NEG, total price
assets:stock -3.00 NEG @@ 6.00 FOO
assets:bank -6.00 FOO
equity:error

Note that without the sign-flipping, x WHATEVER @ y CURRENCY is always equivalent to x WHATEVER @@ x*y CURRENCY, regardless of the signs of x and y. Also x WHATEVER @@ y CURRENCY always balances with -y CURRENCY, so you can simply ignore what's to the left of @@ when balancing those kinds of transactions.
I realize such a drastic change to the journal format would cause severe compatibility issues, but I have some ideas about how to make the transition as painless as possible. I'll be happy to discuss them and help with the coding if my suggestion has a snowball's chance in hell to be implemented in a future version.
(Slightly edited for clarity.)

@Xitian9
Copy link
Collaborator

Xitian9 commented Oct 25, 2022

I'm a mathematician with some understanding of the basics of accounting and some understanding of the internals of hledger. I completely agree with you, and we would probably choose to do that if we were writing this from scratch. Unfortunately, you are correct that we have a lot of legacy to deal with: not only from older versions of hledger, but also with other ledger-likes: e.g., ledger, beancount.

I'd be open to an improvement here, but migration would have to be handled carefully. I'd love to hear your ideas.

There is previous discussion on this issue here: #1509

@tanelihuuskonen
Copy link

Glad to hear we agree in general.
Adding an option to always show the true sign of a total cost in output would be an easy first step. As for input, I see three basic possibilities:

  1. The current way: flip the sign if the quantity is negative.
  2. The proposed new way: never flip the sign.
  3. Accept either of the above.
    Option (3), inferring the correct sign when the input is ambiguous, is possible if we know the sign of the unit price, which can be assumed to be positive unless explicitly declared otherwise, maybe on a per-commodity basis. However, I'm not sure it's worth implementing, as there may be room for subtle bugs and hard-to-find user errors. Just adding a command-line option for (2) is enough for converting existing journals back and forth.

@simonmichael
Copy link
Owner Author

simonmichael commented Oct 25, 2022

More great examples and discussion - thanks!

The current usage of @@ has a certain logic when you read it in the right way. But right now I too agree the above looks more principled, therefore easier to keep straight.

I must still be slightly confused though: @tanelihuuskonen's examples look similar to @Xitian9's june 6th proposed test suite above, but the latter works already with hledger 1.26. I expect I'll see the difference in a minute.

An optional different behaviour, or even change of default behaviour, is not impossible. We should think about whether the reduction in unclarity/confusion would outweigh the new complexity, confusion and churn it would bring.

When someone gets the chance, it would be interesting to also see examples/tests showing the result of --infer-market-price on each of these example transactions, another aspect of this (and the original trigger for this issue).

@tanelihuuskonen
Copy link

@simonmichael : Yes, I agree.
My examples differ from those of @Xitian9 in the cases where you sell a commodity (or pay to get rid of it) and record the total price. I added a dummy equity:error account to those ones to make hledger 1.27.1 accept them. They correspond to @Xitian9 's examples with total prices on 2022-01-01 and 2022-01-03.

@simonmichael
Copy link
Owner Author

simonmichael commented Dec 7, 2022

This has had no impact in my daily usage [since June, I mean; it did impact me then] so I have been unmotivated. Its status as a bug and a regression has felt a bit variable. But it's not good too leave it open so long when it is marked as a regression. Just noting that this issue needs a champion.

@simonmichael
Copy link
Owner Author

simonmichael commented Dec 14, 2022

Here are the tests I'm adding (@Xitian9's examples):

# In fact, here is how sign works with costs currently.
# See discussion at https://github.com/simonmichael/hledger/issues/1870
<
2022-01-01 Positive Unit prices
    a        A 1
    b        B -1 @ A 1

2022-01-01 Positive Total prices
    a        A 1
    b        B -1 @@ A 1

2022-01-02 Negative unit prices
    a        A 1
    b        B 1 @ A -1

2022-01-02 Negative total prices
    a        A 1
    b        B 1 @@ A -1

2022-01-03 Double Negative unit prices
    a        A -1
    b        B -1 @ A -1

2022-01-03 Double Negative total prices
    a        A -1
    b        B -1 @@ A -1

# 23. All these transactions are considered balanced
$ hledger -f- print -x
2022-01-01 Positive Unit prices
    a             A 1
    b      B -1 @ A 1

2022-01-01 Positive Total prices
    a             A 1
    b     B -1 @@ A 1

2022-01-02 Negative unit prices
    a             A 1
    b      B 1 @ A -1

2022-01-02 Negative total prices
    a             A 1
    b     B 1 @@ A -1

2022-01-03 Double Negative unit prices
    a            A -1
    b     B -1 @ A -1

2022-01-03 Double Negative total prices
    a            A -1
    b    B -1 @@ A -1

>=

# 24. Here they are converted to cost
$ hledger -f- print -xB
2022-01-01 Positive Unit prices
    a             A 1
    b            A -1

2022-01-01 Positive Total prices
    a             A 1
    b            A -1

2022-01-02 Negative unit prices
    a             A 1
    b            A -1

2022-01-02 Negative total prices
    a             A 1
    b            A -1

2022-01-03 Double Negative unit prices
    a            A -1
    b             A 1

2022-01-03 Double Negative total prices
    a            A -1
    b             A 1

>=

# 25. Here are the market prices inferred, since 1.26:
$ hledger -f- --infer-market-prices prices
P 2022-01-01 B A 1
P 2022-01-01 B A 1.0
P 2022-01-02 B A -1
P 2022-01-02 B A -1.0
P 2022-01-03 B A -1
P 2022-01-03 B A -1.0

They pass since hledger 1.26. If anyone wants to push @tanelihuuskonen's proposal forward, the next step would be to raise it on the mail list and try to gather support / estimate impact. We could build a test binary and ask people if it breaks with their data. I'm not too keen on adding an option but I suppose we would have to, to ease the transition, or provide a conversion tool.

@simonmichael
Copy link
Owner Author

@simonmichael simonmichael removed the regression A backwards step, indicating a weakness in our QA. We don't like these. label Dec 14, 2022
@simonmichael
Copy link
Owner Author

Closing this as a documentation bug for 1.26's change in market price inference, not a regression. If more happens we can reopen or start a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-BUG Something wrong, confusing or sub-standard in the software, docs, or user experience. docs Documentation-related. valuation
Projects
No open projects
Development

No branches or pull requests

3 participants