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

quantities created using pint.Quantity are inconsistent #1749

Open
keewis opened this issue Apr 21, 2023 · 3 comments
Open

quantities created using pint.Quantity are inconsistent #1749

keewis opened this issue Apr 21, 2023 · 3 comments
Labels

Comments

@keewis
Copy link
Contributor

keewis commented Apr 21, 2023

Consider this:

In [1]: import pint
   ...: 
   ...: q1 = pint.Quantity([0, 1], "m")
   ...: q2 = pint.UnitRegistry.Quantity([0, 1], "m")
   ...: q3 = pint.application_registry.Quantity([0, 1], "m")
   ...: 
   ...: assert q1._REGISTRY is q2._REGISTRY and q1._REGISTRY is q3._REGISTRY
   ...: assert type(q1) == type(q2)
   ...: assert type(q1) == type(q3)
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
Input In [1], in <cell line: 9>()
      7 assert q1._REGISTRY is q2._REGISTRY and q1._REGISTRY is q3._REGISTRY
      8 assert type(q1) == type(q2)
----> 9 assert type(q1) == type(q3)

AssertionError:

We can see that the type returned using pint.Quantity (which is an alias of pint.UnitRegistry.Quantity, i.e. the class member) claims to be created from the application registry but has a different type from quantities directly created from the application registry.

I'm not sure how to debug this, especially since commenting out / relocating the alias definitions for Quantity, Unit, Measurement, and Context in the main module seems to cause the instantiation of _DEFAULT (the LazyRegistry instance) to fail. Any ideas, @hgrecco?


For more context, this has been exposed in xarray's test suite by #1722, before that it would automatically change the registry (and type) to the application registry.

@hgrecco
Copy link
Owner

hgrecco commented Apr 25, 2023

Since the beginning, it was decided that the registry should not be a singleton within the package, but rather a normal object that the user should instantiate. No magic. We later relaxed this idea by creating the default registry and making some magic. In retrospect, not sure that this was a good idea. On top, we now create a specific Quantity class for each type of registry.

One way of solving this might by adding the Quantity class to the application registry within SharedRegistryObject.__new__ in util.py. Basically, changing this:

    def __new__(cls, *args, **kwargs):
        inst = object.__new__(cls)
        if not hasattr(cls, "_REGISTRY"):
            # Base class, not subclasses dynamically by
            # UnitRegistry._init_dynamic_classes
            from . import application_registry

            inst._REGISTRY = application_registry.get()
        return inst

to this:

    def __new__(cls, *args, **kwargs):
        if not hasattr(cls, "_REGISTRY"):
            # Base class, not subclasses dynamically by
            # UnitRegistry._init_dynamic_classes
            from . import application_registry
            cls._REGISTRY = application_registry.get()
        return object.__new__(cls)

or something similar (haven't tested)

In general, I would say that any non-trivial use of Pint within a library should use only the application_registry without keeping a reference to Quantity. i.e. always use application_registry.Quantity, not Q = application_registry.Quantity and then Q(...) as there is no way to invalidate the reference if the application registry is changed. But I would rather consider some more options to avoid adding more complexity and chances of errors. One of such ideas (also no tested) would be to make pint.Quantity, pint.Unit and others special and istead of having a classvar attribute _REGISTRY have a read only property that returns application_registry.get()

What do you think?

@keewis
Copy link
Contributor Author

keewis commented Apr 25, 2023

that is true, but since the recent change to support registries other than the application registry with dask exposed this (before, all types would be changed back to the ones of the application registry), that would mean that there's no common code path for old and new versions of pint, effectively marking pint as never having worked properly with dask, and the new release would be the first one that actually does. I'd be fine with that, we'd just have to be clear about it.

@hgrecco
Copy link
Owner

hgrecco commented Apr 25, 2023

We can support changing the registry as long as dask does not hold a reference directly to Quantity/Unit. Maybe we can review the code together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants