Repository: hgrecco/pint

Issue:
Proper handling of `__array_*__` attributes/methods
After #905 is merged, the following NumPy array protocol attributes/methods will be explicitly handed by Pint Quantities:

- `__array_priority__`
- `__array_ufunc__`
- `__array_function__`

All others (any starting with `__array_`) are implicitly handled by

1) Warning with a `UnitStrippedWarning`
2) If magnitude is an `ndarray`, return the corresponding attribute on the magnitude
3) If the magnitude is not an `ndarray`, return the corresponding attribute on the magnitude converted to an `ndarray`.

Before #905, step 3 above was extremely problematic, since the conversion was done *in place* (see #399, #481, #509). While now it's a bit better because it should no longer mutate the Quantity, the conversion to ndarray may not respect the behavior of arbitrary duck arrays (see #845 / #878), and may return misleading values (such as may be the case with `__array_struct__` and `__array_interface__` as brought up in #905).

Being uncomfortable with this implicit handing, and seeing the issues it has caused, I would suggest that Pint Quantities instead only (explicitly) respond to:

- `__array__` (by warning with `UnitStrippedWarning` and then returning `np.asarray` called on the magnitude)
- `__array_priority__` (see suggestions in https://github.com/hgrecco/pint/pull/905#discussion_r356564550)
- `__array_ufunc__` (as done in #905)
- `__array_function__` (as done in #905)

and raise `AttributeError` for all other `__array_*__` attributes. This would bring Pint into alignment with the recommendations set by NumPy for custom array containers (see https://docs.scipy.org/doc/numpy-1.17.0/user/basics.dispatch.html) and, to the best I can tell, the example set by Dask and Sparse (and to a certain extent CuPy).

However, while it seems to be the "more correct" implementation, given that this would be removing a fallback, it would be a major breaking change with potentially unforeseen consequences. And so, I wanted to reach out for feedback before I try putting together a follow-up PR to #905 to address these issues and work towards resolving #845 / #878.

If this is an acceptable change, would it make sense to include a deprecation cycle, by leaving in the `__array_*__` fallback with a `DeprecationWarning` for Pint v0.10, and then changing to `AttributeError` in v0.11? Or is there a consensus to leave the current behavior in place and try working around the issues that arise?

---

For reference, for the three duck array packages pointed to in [NEP 30](https://numpy.org/neps/nep-0030-duck-array-protocol.html) (Dask, CuPy, and Sparse), here is a brief summary of their behavior with these attributes from what I could scour from their source code (no guarantee on correctness though, as I haven't worked with any of their internals):

**Dask** and **Sparse**

- Explicitly defines
  - `__array__`
  - `__array_priority__`
  - `__array_ufunc__`
  - `__array_function__`
- Does not respond to others

**CuPy**

- Explicitly defines
  - `__array__`
  - `__array_priority__`
  - `__array_ufunc__`
  - `__array_function__`
- `__array_wrap__`, `__array_interface__` listed as TODO in comments
- Has an experimental "fallback mode" in which it would seem to defer to the attribute on the corresponding `ndarray` (see https://github.com/cupy/cupy/blob/1928c3b22c972647ecf2242ddced62015407593f/cupyx/fallback_mode/fallback.py)
- Otherwise does not respond to others
setting warnings to errors changes the behaviour of `np.asarray` on quantities
Using `warnings.filterwarnings` to promote warnings to errors changes the behaviour of `np.asarray` (or rather `np.array` as `np.asarray` delegates to that):
```python
>>> array = np.arange(10) * ureg.m
>>> np.asarray(array)  # warning emitted because units were stripped
[0 1 2 3 4 5 6 7 8 9]
>>> warnings.filterwarnings("error")
>>> np.asarray(array)  # warning should have become an error, but completely disappeared
array([<Quantity(0, 'meter')>, <Quantity(1, 'meter')>,
       <Quantity(2, 'meter')>, <Quantity(3, 'meter')>,
       <Quantity(4, 'meter')>, <Quantity(5, 'meter')>,
       <Quantity(6, 'meter')>, <Quantity(7, 'meter')>,
       <Quantity(8, 'meter')>, <Quantity(9, 'meter')>], dtype=object)
```

This happens with pint `0.9`, but also with the version from #764 (because `__array_function__` does not allow overloading `np.asarray`)

Edit: I do think this is an issue within numpy, but I think it might help to also track this here. Possibly related: numpy/numpy#9761

Discussion:
keewis: I think this also closes #892?
hgrecco: bors r+
bors[bot]: # Merge conflict

Patch:
diff --git a/CHANGES b/CHANGES
index 3f028ea..617d9a4 100644
--- a/CHANGES
+++ b/CHANGES
@@ -5,7 +5,12 @@ Pint Changelog
 -----------------
 
 - Switched test configuration to pytest and added tests of Pint's matplotlib support.
-  (Issue #954)
+  (Issue #954, Thanks Jon Thielen)
+- Deprecate array protocol fallback except where explicitly defined (`__array__`,
+  `__array_priority__`, `__array_function__`, `__array_ufunc__`). The fallback will
+  remain until the next minor version, or if the environment variable
+  `PINT_ARRAY_PROTOCOL_FALLBACK` is set to 0.
+  (Issue #953, Thanks Jon Thielen)
 - Removed eval usage when creating UnitDefinition and PrefixDefinition from string.
   (Issue #942)
 - Added `fmt_locale` argument to registry.
diff --git a/docs/numpy.rst b/docs/numpy.rst
index c4965ee..01098c0 100644
--- a/docs/numpy.rst
+++ b/docs/numpy.rst
@@ -159,6 +159,15 @@ memory and CPU cycles. Therefore, for numerically intensive code, you
 might want to convert the objects first and then use directly the magnitude,
 such as by using Pint's `wraps` utility (see :ref:`wrapping`).
 
+Array interface protocol attributes (such as `__array_struct__` and
+`__array_interface__`) are available on Pint Quantities by deferring to the
+corresponding `__array_*` attribute on the magnitude as casted to an ndarray. This
+has been found to be potentially incorrect and to cause unexpected behavior, and has
+therefore been deprecated. As of the next minor version of Pint (or when the
+`PINT_ARRAY_PROTOCOL_FALLBACK` environment variable is set to 0 prior to importing
+Pint), attempting to access these attributes will instead raise an AttributeError.
+
+
 
 
 
diff --git a/pint/compat.py b/pint/compat.py
index 24fa769..0f68a1d 100644
--- a/pint/compat.py
+++ b/pint/compat.py
@@ -7,6 +7,7 @@
     :copyright: 2013 by Pint Authors, see AUTHORS for more details.
     :license: BSD, see LICENSE for more details.
 """
+import os
 import tokenize
 from decimal import Decimal
 from io import BytesIO
@@ -94,6 +95,8 @@ try:
 
     NP_NO_VALUE = np._NoValue
 
+    ARRAY_FALLBACK = bool(int(os.environ.get("PINT_ARRAY_PROTOCOL_FALLBACK", 1)))
+
 except ImportError:
 
     np = None
@@ -107,9 +110,12 @@ except ImportError:
     HAS_NUMPY_ARRAY_FUNCTION = False
     SKIP_ARRAY_FUNCTION_CHANGE_WARNING = True
     NP_NO_VALUE = None
+    ARRAY_FALLBACK = False
 
     def _to_magnitude(value, force_ndarray=False):
-        if isinstance(value, (dict, bool)) or value is None:
+        if force_ndarray:
+            raise ValueError("Cannot force to ndarray when NumPy is not present.")
+        elif isinstance(value, (dict, bool)) or value is None:
             raise TypeError("Invalid magnitude for Quantity: {0!r}".format(value))
         elif isinstance(value, str) and value == "":
             raise ValueError("Quantity magnitude cannot be an empty string.")
diff --git a/pint/quantity.py b/pint/quantity.py
index 02f8ec5..9f9c6de 100644
--- a/pint/quantity.py
+++ b/pint/quantity.py
@@ -22,6 +22,7 @@ from pkg_resources.extern.packaging import version
 
 from .compat import SKIP_ARRAY_FUNCTION_CHANGE_WARNING  # noqa: F401
 from .compat import (
+    ARRAY_FALLBACK,
     NUMPY_VER,
     BehaviorChangeWarning,
     _to_magnitude,
@@ -1454,6 +1455,14 @@ class Quantity(PrettyIPython, SharedRegistryObject):
         else:
             return value
 
+    def __array__(self):
+        warnings.warn(
+            "The unit of the quantity is stripped when downcasting to ndarray.",
+            UnitStrippedWarning,
+            stacklevel=2,
+        )
+        return _to_magnitude(self._magnitude, force_ndarray=True)
+
     def clip(self, first=None, second=None, out=None, **kwargs):
         minimum = kwargs.get("min", first)
         maximum = kwargs.get("max", second)
@@ -1548,23 +1557,36 @@ class Quantity(PrettyIPython, SharedRegistryObject):
         return len(self._magnitude)
 
     def __getattr__(self, item):
-        # Attributes starting with `__array_` are common attributes of NumPy ndarray.
-        # They are requested by numpy functions.
         if item.startswith("__array_"):
-            warnings.warn(
-                "The unit of the quantity is stripped when getting {} "
-                "attribute".format(item),
-                UnitStrippedWarning,
-                stacklevel=2,
-            )
-            if isinstance(self._magnitude, ndarray):
-                return getattr(self._magnitude, item)
+            # Handle array protocol attributes other than `__array__`
+            if ARRAY_FALLBACK:
+                # Deprecated fallback behavior
+                warnings.warn(
+                    (
+                        f"Array protocol attribute {item} accessed, with unit of the "
+                        "Quantity being stripped. This attribute will become unavailable "
+                        "in the next minor version of Pint. To make this potentially "
+                        "incorrect attribute unavailable now, set the "
+                        "PINT_ARRAY_PROTOCOL_FALLBACK environment variable to 0 before "
+                        "importing Pint."
+                    ),
+                    DeprecationWarning,
+                    stacklevel=2,
+                )
+
+                if isinstance(self._magnitude, ndarray):
+                    return getattr(self._magnitude, item)
+                else:
+                    # If an `__array_` attributes is requested but the magnitude is not an ndarray,
+                    # we convert the magnitude to a numpy ndarray.
+                    # TODO (#905 follow-up): Potentially problematic, investigate for duck arrays
+                    magnitude_as_array = _to_magnitude(
+                        self._magnitude, force_ndarray=True
+                    )
+                    return getattr(magnitude_as_array, item)
             else:
-                # If an `__array_` attributes is requested but the magnitude is not an ndarray,
-                # we convert the magnitude to a numpy ndarray.
-                # TODO (#905 follow-up): Potentially problematic, investigate for duck arrays
-                magnitude_as_array = _to_magnitude(self._magnitude, force_ndarray=True)
-                return getattr(magnitude_as_array, item)
+                # TODO (next minor version): ARRAY_FALLBACK is removed and this becomes the standard behavior
+                raise AttributeError(f"Array protocol attribute {item} not available.")
         elif item in HANDLED_UFUNCS or item in self._wrapped_numpy_methods:
             # TODO (#905 follow-up): Potentially problematic, investigate for duck arrays/scalars
             magnitude_as_array = _to_magnitude(self._magnitude, True)


Test patch:
diff --git a/pint/testsuite/test_numpy.py b/pint/testsuite/test_numpy.py
index b1871f9..c3a17c0 100644
--- a/pint/testsuite/test_numpy.py
+++ b/pint/testsuite/test_numpy.py
@@ -1,8 +1,9 @@
 import copy
 import operator as op
 import unittest
+from unittest.mock import patch
 
-from pint import DimensionalityError, OffsetUnitCalculusError
+from pint import DimensionalityError, OffsetUnitCalculusError, UnitStrippedWarning
 from pint.compat import np
 from pint.testsuite import QuantityTestCase, helpers
 from pint.testsuite.test_umath import TestUFuncs
@@ -1004,6 +1005,26 @@ class TestNumpyUnclassified(TestNumpyMethods):
             np.array([[1, 0, 2], [3, 0, 4]]) * self.ureg.m,
         )
 
+    @patch("pint.quantity.ARRAY_FALLBACK", False)
+    def test_ndarray_downcast(self):
+        with self.assertWarns(UnitStrippedWarning):
+            np.asarray(self.q)
+
+    def test_array_protocol_fallback(self):
+        with self.assertWarns(DeprecationWarning) as cm:
+            for attr in ("__array_struct__", "__array_interface__"):
+                getattr(self.q, attr)
+                warning_text = str(cm.warnings[0].message)
+                self.assertTrue(
+                    f"unit of the Quantity being stripped" in warning_text
+                    and "will become unavailable" in warning_text
+                )
+
+    @patch("pint.quantity.ARRAY_FALLBACK", False)
+    def test_array_protocol_unavailable(self):
+        for attr in ("__array_struct__", "__array_interface__"):
+            self.assertRaises(AttributeError, getattr, self.q, attr)
+
 
 @unittest.skip
 class TestBitTwiddlingUfuncs(TestUFuncs):
diff --git a/pint/testsuite/test_quantity.py b/pint/testsuite/test_quantity.py
index 9f02eac..eecb39e 100644
--- a/pint/testsuite/test_quantity.py
+++ b/pint/testsuite/test_quantity.py
@@ -532,6 +532,10 @@ class TestQuantity(QuantityTestCase):
             warnings.filterwarnings("error")
             self.Q_([])
 
+    @helpers.requires_not_numpy()
+    def test_no_ndarray_coercion_without_numpy(self):
+        self.assertRaises(ValueError, self.Q_(1, "m").__array__)
+
 
 class TestQuantityToCompact(QuantityTestCase):
     def assertQuantityAlmostIdentical(self, q1, q2):