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.

Sign up for GitHub

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

Jump to bottom

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

Open
deanmalmgren opened this issue Feb 26, 2025 · 6 comments · May be fixed by #2147
Open

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

deanmalmgren opened this issue Feb 26, 2025 · 6 comments · May be fixed by #2147

Comments

@deanmalmgren
Copy link

deanmalmgren commented Feb 26, 2025 •
edited
Loading

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
Copy link
Owner

hgrecco commented Feb 26, 2025

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 added a commit to deanmalmgren/pint that referenced this issue Feb 26, 2025
@deanmalmgren
test: added slow/failing test for hgrecco#2146
79f8be0
@deanmalmgren deanmalmgren linked a pull request Feb 26, 2025 that will close this issue
Open
5 tasks
@deanmalmgren
Copy link
Author

✅ Just put a PR together in #2147.

@hgrecco
Copy link
Owner

hgrecco commented Feb 26, 2025

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

@deanmalmgren
Copy link
Author

deanmalmgren commented Feb 26, 2025 •
edited
Loading

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
Copy link
Collaborator

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
Copy link
Owner

hgrecco commented Mar 5, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix 2146: use BFS algorithm for shortest path deanmalmgren/pint
3 participants
@deanmalmgren @hgrecco @andrewgsavage