Surprising sort behaviour for inventories

[yagebu]

When Beancount compares two inventories (__lt__), it compares the sorted lists of positions in those inventories. Positions are sorted by currency (with major currencies first and then by length)) so that leads to the following surprising result:

from beancount.core.inventory import Inventory

inv = Inventory.from_string('10 AA, 10 BB')
inv2 = Inventory.from_string('10 BB')

print(inv > inv2)  # False

I believe the more "correct" thing here would be to ensure that when comparing, we match up positions in the compared inventories. So in the above example, we'd not compare 10 AA to 10 BB but rather to 0 AA.

Since I don't really think that there's a very meaningful total order on all inventories, I'm not too fussed by this, but this could be considered for v3.

[blais]

Great point Jakob, will do

On Sat, Apr 24, 2021, 03:39 Jakob Schnitzer @.***> wrote:

When Beancount compares two inventories (lt), it compares the sorted lists of positions in those inventories. Positions are sorted by currency (with major currencies first and then by length) https://github.com/beancount/beancount/blob/e1716b492c7619682a6d7c33c4873aa41954af1e/beancount/core/position.py#L216) so that leads to the following surprising result:

from beancount.core.inventory import Inventory inv = Inventory.from_string('10 AA, 10 BB')inv2 = Inventory.from_string('10 BB') print(inv > inv2) # False

I believe the more "correct" thing here would be to ensure that when comparing, we match up positions in the compared inventories. So in the above example, we'd not compare 10 AA to 10 BB but rather to 0 AA.

Since I don't really think that there's a very meaningful total order on all inventories, I'm not too fussed by this, but this could be considered for v3.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/beancount/beancount/issues/650, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACSBE7MRVNNYGE6DDMCK53TKJYUPANCNFSM43P34CFQ .

[dnicolodi]

It is also a bit strange that the only comparison operator is __lt__. __eq__ is inherited from dict and __gt__ works for pairs of Inventory objects because Python uses __lt__ with reversed operands, however, __le__ or __ge__ fail with TypeError.

Looking at if from a wider perspective, the issue is indeed that is not possible to define a total ordering for inventories. Applying your definition: 1 CC, 1 AA < 1 BB, 2 CC and 1 BB, 2 CC < 2 BB, 2 AA, which would imply 1 CC, 1 AA < 2 BB, 2 AA however, the last assertion is false in your definition.

Why do we need comparison operators for Inventory objects? The fact that they are only partially implemented suggests that probably we don't need them, thus I think the most sensible thing is to disallow comparisons (which only requires removing the __lt__ method.