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

balance transactions with their amounts' precisions #1479

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

simonmichael
Copy link
Owner

  • lib: move commodityStylesFromAmounts to Hledger.Data.Amount
  • lib: use a transaction's amount precisions when balancing it

@simonmichael simonmichael added the journal The journal file format, and its features. label Feb 6, 2021
@simonmichael
Copy link
Owner Author

"Surprising developments in old behaviour, as a consequence of #931:
now that print shows amounts with all of their decimal places, we had
better balance transactions using all of those visible digits
(so that hledger and a user will agree on whether it's balanced).

So now when transaction balancing compares amounts to see if they look
equal, it uses (for each commodity) the maximum precision seen in just
that transaction's amounts - not the precision from the journal's
commodity display styles. This makes it more localised, which is a
nice simplification.

In journalFinalise, applying commodity display styles to the journal's
amounts is now done as a final step (after transaction balancing, not
before), and only once (rather than twice when auto postings are
enabled), and seems slightly more thorough (affecting some inferred
amounts where it didn't before).

Inferred unit transaction prices (which arise in a two-commodity
transaction with 3+ postings, and can be seen with print -x) may now
be generated with a different number of decimal places than before.
Specifically, it will be the sum of the the number of decimal places
in the amounts being converted to and from. (Whereas before, it was..
some other, perhaps larger number.) Hopefully this will always be a
suitable number of digits such that hledger's & users' calculation of
balancedness will agree.

Lib changes:

Hledger.Data.Journal
added:
journalInferCommodityStyles
journalInferAndApplyCommodityStyles
removed:
canonicalStyleFrom"

@simonmichael simonmichael changed the title txn balancing local precision balance transactions with their amounts' precisions Feb 6, 2021
@simonmichael
Copy link
Owner Author

simonmichael commented Feb 6, 2021

May need a bit more tweaking. It's visibly more picky about unit transaction prices (@), sometimes requiring them to be more precise than previously.

@simonmichael
Copy link
Owner Author

PS here's the original motivation for this: after #931, I hoped that print's output would always be parseable, but sometimes it still wasn't, because transaction balancing was still influenced by commodity declarations, which print currently does not preserve. And secondly now that print is showing all decimal places, it seems simpler and more intuitive for hledger to use those same decimal places when checking transaction balancedness.

@simonmichael
Copy link
Owner Author

simonmichael commented Feb 7, 2021

As mentioned, the new balancing is more picky, requiring more decimals in unit prices for certain transactions (where you have more decimals than usual, eg from investment transactions). It's arguably more correct and simpler (not affected by far-off commodity directives), but this does seem like it would be pretty annoying breakage for folks upgrading. Unfortunately I think this means the old behaviour must be kept around as an option, at least for a while. So I'm thinking possibly:

Add a --balancing=exact|styled option.
exact is the new balancing using the transaction's precisions. (In each commodity, the sum of amounts, when rendered with the max precision of the amounts being summed, looks like exactly zero.)
styled is the old balancing using the global display precisions. (In each commodity, the sum of amounts, when rendered with the journal's inferred/declared display precision, looks like exactly zero.)
exact is the new default. As a migration aid, transaction-balancing errors will suggest correct entries if possible, and mention --balanced=styled as a fallback.
styled is advertised as being deprecated and liable to be dropped eventually.

simonmichael added a commit that referenced this pull request Feb 10, 2021
A surprising development in old behaviour: as a consequence of #931,
print now shows amounts with all of their decimal places, so we had
better balance transactions using all of those visible digits
(so that hledger and a user will agree on whether it's balanced).

So now when transaction balancing compares amounts to see if they look
equal, it uses (for each commodity) the maximum precision seen in just
that transaction's amounts - not the precision from the journal's
commodity display styles.

This makes it more localised - therefore simpler - and more robust,
when print-ing transactions to be re-parsed by hledger (previously,
print-ed transactions could be unparseable because they were dependent
on commodity directives).

However, the new behaviour can break existing journals, so we provide
a `--balancing=exact|styled` option to select the new (default) or old
balancing behaviour. (The old behaviour may not be *perfectly*
replicated, but it's hopefully close enough to be unnoticeable.)
This is intended as a temporary migration aid, hopefully to be removed
eventually.

In journalFinalise, applying commodity display styles to the journal's
amounts is now done as a final step (after transaction balancing, not
before), and only once (rather than twice when auto postings are
enabled), and seems slightly more thorough (affecting some inferred
amounts where it didn't before).

As a consequence of this change, inferred unit transaction
prices (which arise in a two-commodity transaction with 3+ postings,
and can be seen with print -x) may in some cases be generated with a
different (greater) precision than before. Specifically, it will now
be the sum of the number of decimal places in the amounts being
converted to and from. (Whereas before, it was.. something else.)
Hopefully this will always be a suitable number of digits such that
hledger's & users' calculation of balancedness will agree.

Lib changes:

Hledger.Data.Journal
added:
journalInferCommodityStyles
journalInferAndApplyCommodityStyles
removed:
canonicalStyleFrom
@simonmichael
Copy link
Owner Author

simonmichael commented Feb 10, 2021

Repushed with --balancing option, a test and docs. I think it's ready for merge, any review welcome.

"A surprising development in old behaviour: as a consequence of #931,
print now shows amounts with all of their decimal places, so we had
better balance transactions using all of those visible digits
(so that hledger and a user will agree on whether it's balanced).

So now when transaction balancing compares amounts to see if they look
equal, it uses (for each commodity) the maximum precision seen in just
that transaction's amounts - not the precision from the journal's
commodity display styles.

This makes it more localised - therefore simpler - and more robust,
when print-ing transactions to be re-parsed by hledger (previously,
print-ed transactions could be unparseable because they were dependent
on commodity directives).

However, the new behaviour can break existing journals, so we provide
a --balancing=exact|styled option to select the new (default) or old
balancing behaviour. (The old behaviour may not be perfectly
replicated, but it's hopefully close enough to be unnoticeable.)
This is intended as a temporary migration aid, hopefully to be removed
eventually.

In journalFinalise, applying commodity display styles to the journal's
amounts is now done as a final step (after transaction balancing, not
before), and only once (rather than twice when auto postings are
enabled), and seems slightly more thorough (affecting some inferred
amounts where it didn't before).

As a consequence of this change, inferred unit transaction
prices (which arise in a two-commodity transaction with 3+ postings,
and can be seen with print -x) may in some cases be generated with a
different (greater) precision than before. Specifically, it will now
be the sum of the number of decimal places in the amounts being
converted to and from. (Whereas before, it was.. something else.)
Hopefully this will always be a suitable number of digits such that
hledger's & users' calculation of balancedness will agree.

Lib changes:

Hledger.Data.Journal
added:
journalInferCommodityStyles
journalInferAndApplyCommodityStyles
removed:
canonicalStyleFrom"

@apauley
Copy link
Contributor

apauley commented Feb 10, 2021

I ran my standard set of reports using hledger from this branch - I had one transaction from 2018 that was off, and it was easy enough to adjust it.

real postings' sum should be 0 but is: $-0.00000000000000832

@simonmichael
Copy link
Owner Author

simonmichael commented Feb 11, 2021

Here's an entry that's hard to adjust for the new code (minimised from a cost basis adjustment transaction) :

commodity $0.00
2020-01-01
    a     A -11.0 @ $0.093735
    b     A  10.4 @ $0.099143

The amounts are always converted to cost before summing. For --balancing=styled, the above is close enough, since $ amounts are rounded to two decimal places. For --balancing=exact, it fails no matter how precise we make the price, eg:

2020-01-01
    a     A -11.0 @ $0.093735
    b     A  10.4 @ $0.09914278846153846

Why ? Roughly speaking, it converts to cost, transforming the above to:

2020-01-01
    a     AAA -$1.031085
    b     AAA  $1.031084999999999984

It sums these, and would round the result to $'s max precision inferred from the transaction, but there is no precision inferred for $ since the original entry had no $ amounts (price amounts do not influence display styles, remember) so no rounding happens and the result is not zero.

Would we want $ to be rounded to the price amounts' precision here (6 or 17) ? Or to the original commodity's precision (1) ?

@simonmichael
Copy link
Owner Author

simonmichael commented Feb 11, 2021 via email

@simonmichael simonmichael deleted the txn-balancing-local-precision branch February 18, 2021 00:01
@simonmichael simonmichael restored the txn-balancing-local-precision branch February 18, 2021 00:15
@simonmichael simonmichael reopened this Feb 18, 2021
A surprising development in old behaviour: as a consequence of #931,
print now shows amounts with all of their decimal places, so we had
better balance transactions using all of those visible digits
(so that hledger and a user will agree on whether it's balanced).

So now when transaction balancing compares amounts to see if they look
equal, it uses (for each commodity) the maximum precision seen in just
that transaction's amounts - not the precision from the journal's
commodity display styles.

This makes it more localised - therefore simpler - and more robust,
when print-ing transactions to be re-parsed by hledger (previously,
print-ed transactions could be unparseable because they were dependent
on commodity directives).

However, the new behaviour can break existing journals, so we provide
a `--balancing=exact|styled` option to select the new (default) or old
balancing behaviour. (The old behaviour may not be *perfectly*
replicated, but it's hopefully close enough to be unnoticeable.)
This is intended as a temporary migration aid, hopefully to be removed
eventually.

In journalFinalise, applying commodity display styles to the journal's
amounts is now done as a final step (after transaction balancing, not
before), and only once (rather than twice when auto postings are
enabled), and seems slightly more thorough (affecting some inferred
amounts where it didn't before).

As a consequence of this change, inferred unit transaction
prices (which arise in a two-commodity transaction with 3+ postings,
and can be seen with print -x) may in some cases be generated with a
different (greater) precision than before. Specifically, it will now
be the sum of the number of decimal places in the amounts being
converted to and from. (Whereas before, it was.. something else.)
Hopefully this will always be a suitable number of digits such that
hledger's & users' calculation of balancedness will agree.

Lib changes:

Hledger.Data.Journal
added:
journalInferCommodityStyles
journalInferAndApplyCommodityStyles
removed:
canonicalStyleFrom
@simonmichael simonmichael changed the title balance transactions with their amounts' precisions DRAFT: balance transactions with their amounts' precisions Feb 23, 2021
@simonmichael
Copy link
Owner Author

Update: this is stalled, awaiting more testing and fixes for balancing precision issues.

@simonmichael simonmichael marked this pull request as draft February 23, 2021 22:23
@simonmichael simonmichael changed the title DRAFT: balance transactions with their amounts' precisions balance transactions with their amounts' precisions Feb 23, 2021
@simonmichael simonmichael force-pushed the master branch 4 times, most recently from 0e6d4bd to 435e423 Compare July 11, 2021 09:09
@simonmichael simonmichael force-pushed the master branch 4 times, most recently from 56bc295 to 01f9c70 Compare July 11, 2021 09:26
@simonmichael simonmichael added needs:changes To unblock: needs some changes made, in line with recommendations needs:design To unblock: needs more thought/planning, leading to a spec/plan needs:docs To unblock: needs corresponding documentation or doc updates needs:impact-analysis To unblock: needs analysis of interactions with other features, users, ecosystem needs:review To unblock: needs more code/docs/design review by someone labels Aug 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
journal The journal file format, and its features. needs:changes To unblock: needs some changes made, in line with recommendations needs:design To unblock: needs more thought/planning, leading to a spec/plan needs:docs To unblock: needs corresponding documentation or doc updates needs:impact-analysis To unblock: needs analysis of interactions with other features, users, ecosystem needs:review To unblock: needs more code/docs/design review by someone print
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants