Dynamic contexts, currency conversion, and failing to find the shortest path

[deanmalmgren]

First time poster, long-time fan of pint. Thank you.

I'm currently working on a project where I use pint extensively for all kinds of unit conversions. We're working on incorporating a feature to allow for multiple types of currencies and I nearly have a working implementation that plays nicely with pint, but I've run into a strange bug. For reference, the relevant tidbit of the issue is captured in this gist.

When I'm only switching between a few currencies, I notice that it is able to successfully do the calculations without too much trouble. You can see this by running poetry run python working.py, the code is reproduced here for convenience:

import itertools

import pint
import currency_converter

# create a currency converter instance to load all of the data
cc = currency_converter.CurrencyConverter()

# load custom units and instantiate Quantity base class that is used everywhere
ureg = pint.UnitRegistry()
for c in cc.currencies:
    ureg.define(f"{c} = [currency_{c}]")  # i.e. USD = [currency_USD]


# add programmatic context for currency conversion
currency_context = pint.Context("currency", defaults={"date": None})
currencies = list(cc.currencies)  # <------- THIS WORKS WHEN IT IS A SUBSET OF THESE CURRENCIES
for a, b in itertools.combinations(currencies, 2):
    def a2b(_ureg, x, date=None):
        return cc.convert(x.magnitude, a, b, date=date) * _ureg(b)

    def b2a(_ureg, x, date=None):
        return cc.convert(x.magnitude, b, a, date=date) * _ureg(a)

    currency_context.add_transformation(f"[currency_{a}]", f"[currency_{b}]", a2b)
    currency_context.add_transformation(f"[currency_{b}]", f"[currency_{a}]", b2a)
ureg.add_context(currency_context)

Quantity = ureg.Quantity

q = Quantity(1, currencies[0])
with ureg.context("currency"):
    for c in currencies:
        print(q)
        q = q.to(c)
    print(q)
    q = q.to(currencies[0])
    print(q)

But when using all 42 currencies that are supported by currencyconverter as above, I notice that the conversion does not happen. When I KeyboardInterrupt, it appears to be stuck in a pint internal function that is related to the shortest path. Here is the output after running poetry run python hanging.py and then issuing a KeyboardInterrupt after ~5s:

1 LTL
1 LTL
^CTraceback (most recent call last):
  File "/Users/dean/Downloads/pint+currency_converter/hanging.py", line 35, in <module>
    q = q.to(c)
        ^^^^^^^
  File "/Users/dean/Library/Caches/pypoetry/virtualenvs/pint+currency-converter-xbgMmBYE-py3.11/lib/python3.11/site-packages/pint/facets/plain/quantity.py", line 536, in to
    magnitude = self._convert_magnitude_not_inplace(other, *contexts, **ctx_kwargs)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dean/Library/Caches/pypoetry/virtualenvs/pint+currency-converter-xbgMmBYE-py3.11/lib/python3.11/site-packages/pint/facets/plain/quantity.py", line 480, in _convert_magnitude_not_inplace
    return self._REGISTRY.convert(self._magnitude, self._units, other)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dean/Library/Caches/pypoetry/virtualenvs/pint+currency-converter-xbgMmBYE-py3.11/lib/python3.11/site-packages/pint/facets/plain/registry.py", line 1050, in convert
    return self._convert(value, src, dst, inplace)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dean/Library/Caches/pypoetry/virtualenvs/pint+currency-converter-xbgMmBYE-py3.11/lib/python3.11/site-packages/pint/facets/context/registry.py", line 397, in _convert
    path = find_shortest_path(self._active_ctx.graph, src_dim, dst_dim)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dean/Library/Caches/pypoetry/virtualenvs/pint+currency-converter-xbgMmBYE-py3.11/lib/python3.11/site-packages/pint/util.py", line 378, in find_shortest_path
    newpath = find_shortest_path(graph, node, end, path)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dean/Library/Caches/pypoetry/virtualenvs/pint+currency-converter-xbgMmBYE-py3.11/lib/python3.11/site-packages/pint/util.py", line 378, in find_shortest_path
    newpath = find_shortest_path(graph, node, end, path)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dean/Library/Caches/pypoetry/virtualenvs/pint+currency-converter-xbgMmBYE-py3.11/lib/python3.11/site-packages/pint/util.py", line 378, in find_shortest_path
    newpath = find_shortest_path(graph, node, end, path)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  [Previous line repeated 35 more times]
  File "/Users/dean/Library/Caches/pypoetry/virtualenvs/pint+currency-converter-xbgMmBYE-py3.11/lib/python3.11/site-packages/pint/util.py", line 377, in find_shortest_path
    if node not in path:
       ^^^^^^^^^^^^^^^^
  File "/Users/dean/Library/Caches/pypoetry/virtualenvs/pint+currency-converter-xbgMmBYE-py3.11/lib/python3.11/site-packages/pint/util.py", line 822, in __eq__
    return self.scale == other.scale and super().__eq__(other)
                                         ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dean/Library/Caches/pypoetry/virtualenvs/pint+currency-converter-xbgMmBYE-py3.11/lib/python3.11/site-packages/pint/util.py", line 581, in __eq__
    if UnitsContainer.__hash__(self) != UnitsContainer.__hash__(other):
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/dean/Library/Caches/pypoetry/virtualenvs/pint+currency-converter-xbgMmBYE-py3.11/lib/python3.11/site-packages/pint/util.py", line 561, in __hash__
    def __hash__(self) -> int:

KeyboardInterrupt

My best guess is that there is a gap in the shortest path algorithm that is being used to infer the unit conversion. I am specifying all pairwise conversions so there really shouldn't be any need for anything terribly fancy to infer the conversion path. Looking at the implementation of util.find_shortest_path, it made me wonder if a breadth first search approach would be more efficient.

Thanks in advance for your help!

[hgrecco]

Welcome and thanks for words about Pint.

I think you are right. For the most of the graphs of units that I can think of BFS should be faster. Would you like to give it a try and if it works make a PR

[deanmalmgren]

✅ Just put a PR together in #2147.

[hgrecco]

Great, I will take a look. Do you have a benchmark?

[deanmalmgren]

I don't have a benchmark, but I did add another unit test that is very slow without the improvement that mimics the situation I was in.

79f8be0

[andrewgsavage]

This is the first time I have seen a robust way of setting up daily currency rates in bulk. Is this something worth adding to pint's documentation or potentially to pint itself or a pint-currency module?

[hgrecco]

@andrewgsavage I really like this idea. We have received this question about currencies many times in the last years.

[deanmalmgren]

@andrewgsavage now that #2147 is merged in, it should be possible to incorporate currency conversion in a scalable way like this. I'm not sure of how to add a pint-currency module or where it might make sense to document a solution like this.

[andrewgsavage]

from your gist,

this part

ureg = pint.UnitRegistry()
for c in cc.currencies:
    ureg.define(f"{c} = [currency_{c}]")  # i.e. USD = [currency_USD]


# add programmatic context for currency conversion
currency_context = pint.Context("currency", defaults={"date": None})
currencies = ["USD", "EUR", "JPY", "DKK", "CAD"]
for a, b in itertools.combinations(currencies, 2):
    def a2b(_ureg, x, date=None, a=a, b=b):
        return cc.convert(x.magnitude, a, b, date=date) * _ureg(b)

    def b2a(_ureg, x, date=None, a=a, b=b):
        return cc.convert(x.magnitude, b, a, date=date) * _ureg(a)

    currency_context.add_transformation(f"[currency_{a}]", f"[currency_{b}]", a2b)
    currency_context.add_transformation(f"[currency_{b}]", f"[currency_{a}]", b2a)
ureg.add_context(currency_context)

could be added to pint as a function, eg UnitRegistry.add_currency_conversion(cc, currencies)

or we can add your gist directly to the docs on the currency page

there's not much code so it doesn't seem worthwhile making a module for it