dascore/utils/patch.py (1)

626-631: Tightened error-message formatting — OK.

String tweak looks good and keeps the message compact.

While here, there are tiny doc typos nearby you may want to fix later: “nane” → “name” (Line 568) and “retreiving” → “retrieving” (Line 593).

dascore/constants.py (1)

5-5: Centralizing _AGG_FUNCS — good; consider freezing and typing.

The mapping and partial(np.take,…) choices are appropriate for axis-aware reductions.

• Add a read-only wrapper to prevent accidental mutation:
  • from types import MappingProxyType
  • _AGG_FUNCS = MappingProxyType(_AGG_FUNCS)
• Add a type hint for clarity (e.g., dict[str, Callable]): minor but improves IDE help.

Also applies to: 190-199

tests/test_proc/test_basic.py (1)

167-176: test_max uses L1 norm and sum — fix to use max norm.

The “max” normalization test should use norm="max" and check max(abs(.)) per axis.

Apply this diff:

-        # test along the distance axis
-        dist_norm = random_patch.normalize("distance", norm="l1")
+        # test along the distance axis
+        dist_norm = random_patch.normalize("distance", norm="max")
         axis = dims.index("distance")
-        norm = np.abs(np.sum(dist_norm.data, axis=axis))
+        norm = np.max(np.abs(dist_norm.data), axis=axis)
         assert np.allclose(norm, 1)
         # tests along time axis
-        time_norm = random_patch.normalize("time", norm="l1")
+        time_norm = random_patch.normalize("time", norm="max")
         axis = dims.index("time")
-        norm = np.abs(np.sum(time_norm.data, axis=axis))
+        norm = np.max(np.abs(time_norm.data), axis=axis)
         assert np.allclose(norm, 1)

Also, optional: the class name “TestStandarize” has a typo; “TestStandardize” reads better.

dascore/proc/aggregate.py (1)

85-87: Single aggregation path — OK.

Delegating to _apply_aggregator keeps behavior consistent across methods.

For deterministic docs, consider sorted(_AGG_FUNCS) instead of list(_AGG_FUNCS) in the compose_docstring options.

dascore/proc/basic.py (1)

156-165: bool_patch behavior is correct; tighten docstring.

Minor nits: “on” → “one”; “value error” → “ValueError”.

-    This follows the numpy convention of raising a value error if the patch
-    has more than on element. Otherwise, it returns the truthy value of the
+    This follows the NumPy convention of raising a ValueError if the patch
+    has more than one element. Otherwise, it returns the truthiness of the
     one element.

dascore/core/patch.py (4)

237-241: Fix dtype annotation type.

dtype returns a numpy dtype, not tuple[int, ...].

-    def dtype(self) -> tuple[int, ...]:
+    def dtype(self) -> np.dtype:

---

233-236: Fix size annotation type.

size is an int.

-    def size(self) -> tuple[int, ...]:
+    def size(self) -> int:

---

252-256: Remove unused blanket noqa or scope it.

Ruff flagged unused blanket noqa. Either remove it or target the rule explicitly if you’re silencing N802 for the .T alias.

-    def T(self):  # noqa
+    def T(self):  # noqa: N802

---

189-189: Remove stray pass.

Dead statement; safe to drop.

-        pass

dascore/utils/array.py (5)

137-144: Preserve original exception context for unit errors.

Use “raise … from err”.

-        except DimensionalityError:
+        except DimensionalityError as err:
             msg = f"{operator} failed with units {data_units} and {other.units}"
-            raise UnitError(msg)
+            raise UnitError(msg) from err

---

213-217: Avoid bare except; narrow to expected numeric/ufunc failures.

Catching Exception masks bugs. Limit to common NumPy errors.

-        except Exception:  # Fall back to floats and re-packing.
+        except (TypeError, ValueError, np.core._exceptions.UFuncTypeError):
             float_data = dc.to_float(data)

---

243-251: Silence unused loop var.

value isn’t used; rename to underscore.

-    for dim, axis, value in dfo:
+    for dim, axis, _ in dfo:

---

151-151: Typo in assertion message.

DASCOre → DASCore.

-    assert len(args) <= 2, "Currently, DASCOre ufuncs are only supported for 2 inputs."
+    assert len(args) <= 2, "Currently, DASCore ufuncs are only supported for 2 inputs."

---

335-342: array_function reassembly: add fallback when no ‘axis’ is present.

Functions without an axis parameter (e.g., np.square) will return None here. Consider returning patch.new(data=result) if shapes match; otherwise raise a clear error.

-    if "axis" in sig.parameters:
+    if "axis" in sig.parameters:
         dims, inds = _get_dims_and_inds_from_signature(patch, sig, args, kwargs)
         # re-expand array.
         result = result[inds]
         new_coords = {x: _get_new_coord(patch.get_coord(x)) for x in dims}
         cm = patch.coords.update(**new_coords)
         return patch.new(data=result, coords=cm)
+    # Fallback: if shape unchanged, just wrap result; else fail clearly.
+    if result.shape == patch.shape:
+        return patch.new(data=result)
+    msg = f"Cannot reassemble result of {getattr(func, '__name__', func)} without an axis parameter."
+    raise ParameterError(msg)

tests/test_utils/test_array_utils.py (8)

30-37: Strengthen the “incompatible coords” assertions.

Also assert zero-sized data and stable dims for clearer failure signals.

Apply this diff:

         out = apply_ufunc(np.multiply, new, random_patch)
-        assert 0 in set(out.shape)
+        assert 0 in set(out.shape)
+        assert out.data.size == 0
+        assert out.dims == random_patch.dims

---

53-56: Normalize dimensionless-unit assertion.

Compare via get_quantity and accept explicit “1” or omitted units; drop the redundant ndarray type check.

Apply this diff:

-        new = apply_ufunc(np.divide, patch, other)
-        new_units = get_quantity("m/s") / get_quantity("m/s")
-        assert new.attrs.data_units is None or new.attrs.data_units == new_units
-        assert isinstance(new.data, np.ndarray)
+        new = apply_ufunc(np.divide, patch, other)
+        # Dimensionless result: units may be omitted; accept None or "1".
+        q = get_quantity(new.attrs.data_units)
+        assert q is None or q == get_quantity("1")

---

73-76: Same dimensionless-unit normalization here.

Apply this diff:

-        new = apply_ufunc(np.divide, patch, other)
-        new_units = get_quantity("m/s") / get_quantity("m/s")
-        assert new.attrs.data_units is None or new.attrs.data_units == new_units
-        assert np.allclose(new.data, random_patch.data)
+        new = apply_ufunc(np.divide, patch, other)
+        q = get_quantity(new.attrs.data_units)
+        assert q is None or q == get_quantity("1")
+        assert np.allclose(new.data, random_patch.data)

---

46-46: Remove redundant ndarray type checks.

These don’t add signal; failures will show up in the numeric assertions.

Apply this diff:

-        assert isinstance(new.data, np.ndarray)

(Repeat for the two other identical occurrences.)

Also applies to: 51-51, 56-56

---

97-97: Avoid relying on get_quantity(Unit) coercion.

Prefer string form to reduce dependence on permissive parsing.

Apply this diff:

-        assert get_quantity(out1.attrs.data_units) == get_quantity(furlongs)
+        assert get_quantity(out1.attrs.data_units) == get_quantity("furlongs")

---

131-131: Be explicit about comparing coord keys.

Small clarity bump; avoids relying on Mapping’s iteration semantics.

Apply this diff:

-        assert set(out.coords.coord_map) == set(random_dft_patch.coords.coord_map)
+        assert set(out.coords.coord_map.keys()) == set(random_dft_patch.coords.coord_map.keys())

---

24-29: Add reversed-operand protocol tests (NumPy on LHS).

Verifies array_ufunc/array_function when Patch is rhs.

Add:

+    def test_reversed_scalar(self, random_patch):
+        out = 10 + random_patch  # np.add with Patch on RHS
+        assert np.allclose(out.data, random_patch.data + 10)
+
+    def test_reversed_array_like(self, random_patch):
+        ones = np.ones(random_patch.shape)
+        out = ones * random_patch
+        assert np.allclose(out.data, ones * random_patch.data)
+
+    def test_reversed_unit_and_quantity(self, random_patch):
+        pa = random_patch.set_units("m/s")
+        out1 = (m / s) + pa          # unit on LHS
+        out2 = (10 * m / s) + pa     # quantity on LHS
+        assert np.allclose(out1.data, random_patch.data + 1)
+        assert np.allclose(out2.data, random_patch.data + 10)

---

16-132: Optional: parametrize repeated unit-op cases.

Would reduce duplication and produce tighter failure diffs.

Example:

@pytest.mark.parametrize(
    "op, other, expected_units, expected_data",
    [
        (np.multiply, 10 * m / s, "m**2/s**2", lambda d: d),
        (np.add,      10 * m / s, "m/s",        lambda d: d + 10),
        (np.divide,   10 * m / s, "1",          lambda d: d),
    ],
)
def test_quantity_ops_param(self, random_patch, op, other, expected_units, expected_data):
    pa = random_patch.set_units("m/s")
    out = apply_ufunc(op, pa, other)
    q = get_quantity(out.attrs.data_units)
    assert (q is None and expected_units == "1") or q == get_quantity(expected_units)
    assert np.allclose(out.data, expected_data(random_patch.data))

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

tests/test_proc/test_basic.py (1)

164-177: Duplicate test method test_l1_norm.

This method duplicates the logic of test_l1 on lines 150-162. Both test the same L1 normalization functionality with identical assertions.

This duplication should be removed. Apply this diff to fix:

-    def test_l1_norm(self, random_patch):
-        """Ensure after l1 normalization operation norms are 1."""
-        dims = random_patch.dims
-        # test along the distance axis
-        dist_norm = random_patch.normalize("distance", norm="l1")
-        axis = dims.index("distance")
-        norm = np.abs(np.sum(dist_norm.data, axis=axis))
-        assert np.allclose(norm, 1)
-        # tests along time axis
-        time_norm = random_patch.normalize("time", norm="l1")
-        axis = dims.index("time")
-        norm = np.abs(np.sum(time_norm.data, axis=axis))
-        assert np.allclose(norm, 1)

dascore/core/patch.py (1)

250-254: Remove unused noqa directive.

The noqa: N802 directive on line 251 is unnecessary and should be removed as indicated by the static analysis tool.

-    def T(self):  # noqa: N802
+    def T(self):

dascore/utils/array.py (3)

134-154: Consider improving error messages in unit operations.

The unit error handling in _apply_op_units could be more informative by including the actual units in the error message.

        except DimensionalityError as er:
-            msg = f"{operator} failed with units {data_units} and {other.units}"
+            units_str = f"{data_units} and {getattr(other, 'units', 'unknown')}"
+            msg = f"Cannot apply {operator.__name__} to incompatible units: {units_str}"
            raise UnitError(msg) from er

---

437-442: Unused self parameter in protocol methods.

Both patch_array_ufunc and patch_array_function have an unused self parameter. While this is expected for protocol compliance, consider adding a comment to clarify this.

 def patch_array_ufunc(self, ufunc, method, *inputs, **kwargs):
     """
     Called when a numpy ufunc is applied to a patch (__array_ufunc__).
+    
+    The self parameter is required for protocol compliance but not used directly.
     """
     method = ufunc if method == "__call__" else getattr(ufunc, method, ufunc)
     return apply_ufunc(method, *inputs, **kwargs)

 def patch_array_function(self, func, types, args, kwargs):
     """
     Intercept NumPy functions for patch operations.
 
     Parameters
     ----------
+    self : Patch
+        The patch instance (required for protocol but not used).
     func : callable
         The NumPy function being called.

---

324-356: Consider simplifying the patch reassembly logic.

The _reassemble_patch function has complex fallback logic for handling missing signatures. Consider extracting the signature retrieval into a separate helper function for clarity.

+def _get_function_signature(func):
+    """Get the signature of a function, with fallback for ufunc methods."""
+    try:
+        return inspect.signature(func)
+    except (ValueError, AttributeError):
+        ufunc_method_name = {"reduce": _dummy_reduce, "accumulate": _dummy_accumulate}
+        name = getattr(func, "__name__", None)
+        if name in ufunc_method_name:
+            return inspect.signature(ufunc_method_name[name])
+        raise
+
 def _reassemble_patch(result, patch, func, args, kwargs):
     """
     Method to put the patch back together.
     """
     # Simples case, data shape hasn't changed.
     if result.shape == patch.shape:
         return patch.new(data=result)
     # Otherwise, we have to do some detective work to figure out what happened
     # to the array so we can adjust the coords accordingly.
-    try:
-        sig = inspect.signature(func)
-    # For some reason, some ufuncs methods like reduce don't have signatures.
-    except (ValueError, AttributeError):
-        ufunc_method_name = {"reduce": _dummy_reduce, "accumulate": _dummy_accumulate}
-        name = getattr(func, "__name__", None)
-        assert name in ufunc_method_name
-        sig = inspect.signature(ufunc_method_name[name])
+    sig = _get_function_signature(func)

tests/test_utils/test_array_utils.py (2)

21-26: Fix typo in test method name.

The test method is named test_uniary_ufuncs but should be test_unary_ufuncs.

-    def test_uniary_ufuncs(self, func, random_patch):
+    def test_unary_ufuncs(self, func, random_patch):

---

89-94: Consider tightening the dimensionless unit assertion.

The test accepts either None or "1" for dimensionless units, but it would be cleaner to have consistent behavior.

         # Dimensionless result: units may be omitted; accept None or "1".
         q = get_quantity(new.attrs.data_units)
-        assert q is None or q == get_quantity("1")
+        # Prefer consistent behavior - either always None or always "1" for dimensionless
+        assert q is None or str(q) == "1" or q == get_quantity("1")

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

dascore/utils/array.py (2)

122-133: Consider more helpful error message for broadcasting failures.

The error message could be more informative by including the actual shapes.

         if (patch_ndims := patch.ndim) < (array_ndims := other.ndim):
-            msg = f"Cannot broadcast patch/array {patch_ndims=} {array_ndims=}"
+            msg = (
+                f"Cannot broadcast patch (shape={patch.shape}) with array "
+                f"(shape={other.shape}): patch has fewer dimensions"
+            )
             raise PatchBroadcastError(msg)

---

339-339: Remove redundant assertion.

This assertion is redundant with the one on line 337.

     assert len(patches), "No patches found in apply_array_func"
     first_patch = patches[0]
-    assert len(patches) > 0
     # Call the array function

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

tests/test_utils/test_patch_utils.py (1)

651-700: Solid coverage for dim_to_axis; add one negative test.
The positive cases are covered well (list, None, passthrough, single str). Consider adding a test that passes an unknown dim (e.g., "foo") to assert it raises ValueError from list.index, which clarifies failure semantics.

+    def test_dim_to_axis_unknown_dim_raises(self, random_patch):
+        args = ()
+        kwargs = {"dim": "foo"}
+        with pytest.raises(ValueError):
+            dim_to_axis(random_patch, args, kwargs)

docs/tutorial/patch.qmd (4)

637-677: Clarify numpy-like operations wording.
“Applied directly to a patch using built-in python operators” is accurate for operators/ufuncs. Consider adding one sentence that non-ufunc numpy functions (routines routed via array_function) require axis, and DASCore keeps reduced dimensions with length 1 by default.

---

681-693: State the protocol names explicitly.
Since this section introduces the protocol, add “(array_ufunc and array_function)” to make it explicit for users scanning docs.

---

715-748: Broadcast example: show shape before/after.
Briefly print shapes to reinforce broadcasting behavior for readers.

---

750-803: PatchUFunc section reads well; add note on dim vs axis.
Mention that PatchUFunc.reduce/accumulate accept dim (mapped internally to axis), while numpy ufuncs accept axis.

dascore/utils/array.py (4)

140-153: Unit-error message may crash if other lacks .units.
When DimensionalityError occurs and other is a Unit without .units (or plain scalar), f-string will raise AttributeError. Guard via getattr.

-        except DimensionalityError as er:
-            msg = f"{operator} failed with units {data_units} and {other.units}"
+        except DimensionalityError as er:
+            other_units = getattr(other, "units", None)
+            msg = f"{operator} failed with units {data_units} and {other_units}"
             raise UnitError(msg) from er

---

121-133: Edge-case: array has more dims than patch.
Raising immediately if array_ndims > patch_ndims is strict; broadcasting can still succeed when leading dims are length-1 after make_broadcastable_to. Consider deferring to make_broadcastable_to or numpy.broadcast_shapes and only raise on failure.

-        if (patch_ndims := patch.ndim) < (array_ndims := other.ndim):
-            msg = f"Cannot broadcast patch/array {patch_ndims=} {array_ndims=}"
-            raise PatchBroadcastError(msg)
-        patch = patch.make_broadcastable_to(other.shape)
+        try:
+            # Let Patch decide how to align; raise if not possible.
+            patch = patch.make_broadcastable_to(other.shape)
+        except Exception as er:
+            raise PatchBroadcastError(
+                f"Cannot broadcast patch of shape {patch.shape} to {other.shape}"
+            ) from er

---

509-515: Out parameter unsupported: fail fast with clear error.
NumPy passes out= arrays to array_ufunc. Since we always construct a new Patch, reject out to avoid silent misuse.

 def patch_array_ufunc(self, ufunc, method, *inputs, **kwargs):
@@
-    method = ufunc if method == "__call__" else getattr(ufunc, method, ufunc)
+    if "out" in kwargs and kwargs["out"] is not None:
+        raise ParameterError("The 'out' kwarg is not supported for Patch ufuncs.")
+    method = ufunc if method == "__call__" else getattr(ufunc, method, ufunc)
     return apply_ufunc(method, *inputs, **kwargs)

---

517-535: Prefer NotImplemented over assert for array_function.
Asserting a Patch type is involved can crash in unexpected dispatch paths. Return NotImplemented to let NumPy fall back when appropriate.

-    # Only handle functions involving Patches
-    assert any(issubclass(t, dc.Patch) for t in types)
-    return apply_array_func(func, *args, **kwargs)
+    if any(issubclass(t, dc.Patch) for t in types):
+        return apply_array_func(func, *args, **kwargs)
+    return NotImplemented

dascore/core/patch.py (1)

250-255: .T property is fine; consider dropping unused noqa.
If the linter isn’t enforcing N802, the noqa can be removed. Otherwise keep as-is.

tests/test_utils/test_array_utils.py (6)

21-26: Typo in test name; add dtype check for boolean ufuncs.

Rename “uniary” → “unary”. Also assert bool dtype for np.isfinite to harden behavior.

Apply this diff:

-    @pytest.mark.parametrize("func", (np.abs, np.tan, np.isfinite, np.exp))
-    def test_uniary_ufuncs(self, func, random_patch):
+    @pytest.mark.parametrize("func", (np.abs, np.tan, np.isfinite, np.exp))
+    def test_unary_ufuncs(self, func, random_patch):
         """Ensure ufuncs that take a single input also work."""
         out = func(random_patch)
         assert isinstance(out, dc.Patch)
+        if func is np.isfinite:
+            assert out.data.dtype == np.bool_

---

50-58: Make xfail strict to catch future XPASS when Pint interop improves.

This guards against the test silently “passing” when the upstream behavior changes.

-    @pytest.mark.xfail(raises=(UnitError, DimensionalityError))
+    @pytest.mark.xfail(raises=(UnitError, DimensionalityError), strict=True)
     def test_reversed_unit_and_quantity(self, random_patch):

---

111-114: Normalize dimensionless-units assertion (use get_quantity on both sides).

Current check mixes raw attr with a Quantity; align with earlier pattern to avoid false negatives.

-        new = apply_ufunc(np.divide, patch, other)
-        new_units = get_quantity("m/s") / get_quantity("m/s")
-        assert new.attrs.data_units is None or new.attrs.data_units == new_units
-        assert np.allclose(new.data, random_patch.data)
+        new = apply_ufunc(np.divide, patch, other)
+        # Dimensionless result: units may be omitted; accept None or "1".
+        q = get_quantity(new.attrs.data_units)
+        assert q is None or q == get_quantity("1")
+        assert np.allclose(new.data, random_patch.data)

---

169-173: Remove redundant assertion.

set(mapping) already yields the keys; the second check duplicates the first.

-        assert out_coord_keys == input_coord_keys
-        assert set(out.coords.coord_map) == set(random_dft_patch.coords.coord_map)
+        assert out_coord_keys == input_coord_keys

---

466-471: Silence Ruff ARG001 by marking the unused param.

The argument is intentionally unused; prefix with underscore (and optionally keep a noqa).

-        def shape_changingFunc(data):
+        def shape_changing_func(_data):  # noqa: ARG001
             # Return a different shape to trigger the error path
             return np.array([1, 2, 3])  # Always return same small array

---

27-37: Add direct tests for boolean comparisons (core PR objective).

Missing explicit checks for patch > scalar, scalar > patch, <=, >=, and dtype=bool. Suggest adding below in TestApplyUfunc.

+    def test_boolean_comparisons(self, random_patch):
+        pa = random_patch
+        gt = pa > 0
+        assert isinstance(gt, dc.Patch)
+        assert gt.data.dtype == np.bool_
+        # reversed
+        rgt = 0 < pa
+        assert rgt.equals(gt)
+        # equality across self should be all True for <= and >=
+        assert np.all((pa <= pa).data)
+        assert np.all((pa >= pa).data)
+
+    def test_boolean_comparisons_with_units(self, random_patch):
+        pa = random_patch.set_units("m/s")
+        q = 10 * m / s
+        out = pa > q
+        assert isinstance(out, dc.Patch)
+        assert out.data.dtype == np.bool_

If you want, I can open a follow-up PR with these tests.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

dascore/utils/array.py (1)

180-183: Bug: invalid isinstance union — raises at runtime

isinstance(other, Quantity | Unit) raises TypeError. Use a tuple of types.

Apply this diff:

-    if isinstance(other, Quantity | Unit):
+    if isinstance(other, (Quantity, Unit)):
         new_data, attrs = _apply_op_units(patch, other, operator, attrs, reversed)
     else:
         new_data = _apply_op(patch.data, other, operator, reversed)

dascore/utils/patch.py (1)

1183-1234: Handle integer dim, remove inner imports, and tighten typing in swap_kwargs_dim_to_axis

• Passing dim as an int currently fails (TypeError: 'int' object is not iterable). Support ints by forwarding them directly to axis.
• Avoid importing ParameterError inside the loop; it’s already imported at module scope.
• Optional: add type hints for better API clarity and future linting.

Apply this diff:

-def swap_kwargs_dim_to_axis(patch, kwargs):
+def swap_kwargs_dim_to_axis(patch: PatchType, kwargs: Mapping[str, Any]) -> dict:
@@
-    if dim is not None:
-        if isinstance(dim, str):
+    if dim is not None:
+        if isinstance(dim, str):
             if dim not in patch.dims:
-                from dascore.exceptions import ParameterError
-
                 msg = f"Dimension '{dim}' not found in patch dimensions {patch.dims}"
                 raise ParameterError(msg)
             axis = patch.dims.index(dim)
-        else:
+        elif isinstance(dim, Sequence) and not isinstance(dim, (str, bytes)):
             # Handle sequence of dimensions
             axis = []
             for d in dim:
                 if d not in patch.dims:
-                    from dascore.exceptions import ParameterError
-
                     msg = f"Dimension '{d}' not found in patch dimensions {patch.dims}"
                     raise ParameterError(msg)
                 axis.append(patch.dims.index(d))
+        elif isinstance(dim, int):
+            axis = dim
+        else:
+            raise ParameterError(f"Unsupported dim type: {type(dim)!r}")
         new_kwargs["axis"] = axis

Also, in the example comment, rename new_kwarg to new_kwargs for consistency.

docs/tutorial/patch.qmd (3)

681-683: Wording: clarify NumPy integration terminology

“implements the numpy array protocol” can be misread as array (conversion). This section is about duck-array protocols (array_function, array_ufunc). Consider “Patch integrates with NumPy’s duck-array protocols (array_function, array_ufunc).”

---

753-756: Rename apply_array_ufunc → apply_ufunc in text

The function is named apply_ufunc; the text says apply_array_ufunc. Update wording to avoid confusion; the link target is already correct.

- DASCore uses [apply_array_ufunc](`dascore.utils.array.apply_ufunc`) and [array_function](`dascore.utils.array.apply_array_func`)
+ DASCore uses [apply_ufunc](`dascore.utils.array.apply_ufunc`) and [`apply_array_func`](`dascore.utils.array.apply_array_func`)

---

785-787: Note formatting

Use consistent code formatting for names: PatchUFunc.reduce/accumulate are methods; surround with backticks to match surrounding style.

tests/test_core/test_patch.py (1)

792-858: Great NumPy interoperability coverage

Exercises reducers, accumulators, full reductions, multi-axis, and bound PatchUFunc paths; also tests ufunc.at error. Consider adding a keepdims=True case for reducers.

dascore/utils/array.py (3)

523-531: Silence ARG001 by marking self unused in patch_array_ufunc

Rename the parameter to _self to match its unused role and satisfy linters.

-def patch_array_ufunc(self, ufunc, method, *inputs, **kwargs):
+def patch_array_ufunc(_self, ufunc, method, *inputs, **kwargs):

---

532-550: Silence ARG001 by marking self unused in patch_array_function

Same as above; rename to _self.

-def patch_array_function(self, func, types, args, kwargs):
+def patch_array_function(_self, func, types, args, kwargs):

---

439-457: Multiple-patch array functions not supported (FYI)

apply_array_func uses the first Patch for reassembly and dim→axis mapping. That’s fine for reducers (single-operand), but document/guard multi-Patch calls via array_function if ever exposed.

tests/test_utils/test_array_utils.py (1)

467-470: Remove unused parameter to fix linter warning.

The data parameter in the shape_changing_func is unused, which triggers a static analysis warning.

Apply this diff to fix the linter warning:

-        def shape_changing_func(data):
+        def shape_changing_func(_):
             # Return a different shape to trigger the error path
             return np.array([1, 2, 3])  # Always return same small array

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

dascore/utils/array.py (1)

180-183: Critical: isinstance with union type raises at runtime.

isinstance(other, Quantity | Unit) throws TypeError. Use a tuple.

-    if isinstance(other, Quantity | Unit):
+    if isinstance(other, (Quantity, Unit)):
         new_data, attrs = _apply_op_units(patch, other, operator, attrs, reversed)
     else:
         new_data = _apply_op(patch.data, other, operator, reversed)

dascore/utils/array.py (2)

526-533: Silence unused self warnings without changing behavior.

Minor lint-only tweak to satisfy Ruff ARG001.

 def patch_array_ufunc(self, ufunc, method, *inputs, **kwargs):
     """
     Called when a numpy ufunc is applied to a patch (__array_ufunc__).
     """
-    _raise_on_out(kwargs)
+    # `self` is unused by design in this dispatcher.
+    del self
+    _raise_on_out(kwargs)

---

535-553: Same minor lint fix for __array_function__.

 def patch_array_function(self, func, types, args, kwargs):
     """
     Intercept NumPy functions for patch operations.
     """
-    # Only handle functions involving Patches
-    _raise_on_out(kwargs)
+    # Only handle functions involving Patches
+    del self
+    _raise_on_out(kwargs)

tests/test_utils/test_array_utils.py (1)

460-465: Add tests to lock in unitless results for boolean ops.

To prevent regressions, assert that boolean outputs clear data_units for both unary and binary cases.

@@
 class TestApplyUfunc:
@@
     def test_out_parameter_raises(self, random_patch):
         """Since patches are immutable, we cant support out. Raise if provided."""
         match = "cannot be used"
         with pytest.raises(ParameterError, match=match):
             apply_ufunc(np.add, random_patch, random_patch, out=random_patch)
+
+    def test_binary_comparison_drops_units(self, random_patch):
+        """Boolean results must be unitless."""
+        pa = random_patch.set_units("m")
+        out = apply_ufunc(np.greater, pa, 2.0)
+        assert out.data.dtype == np.bool_
+        assert out.attrs.data_units is None
+
+    def test_unary_boolean_drops_units(self, random_patch):
+        """Unary boolean ufuncs must be unitless."""
+        pa = random_patch.set_units("s")
+        out = np.isfinite(pa)
+        assert out.data.dtype == np.bool_
+        assert out.attrs.data_units is None

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

dascore/utils/array.py (4)

180-183: Fix invalid isinstance union check (runtime TypeError).

isinstance(other, Quantity | Unit) raises at runtime; use a tuple.

-    if isinstance(other, Quantity | Unit):
+    if isinstance(other, (Quantity, Unit)):
         new_data, attrs = _apply_op_units(patch, other, operator, attrs, reversed)
     else:
         new_data = _apply_op(patch.data, other, operator, reversed)

---

359-362: Fix invalid isinstance(axis, str | bytes) (runtime TypeError).

Use a tuple for the second arg to isinstance.

-        if isinstance(axis, Iterable) and not isinstance(axis, str | bytes):
+        if isinstance(axis, Iterable) and not isinstance(axis, (str, bytes)):
             axes = tuple(int(a) for a in axis)
         else:
             axes = (int(axis),)

---

62-64: Boolean unary ufuncs must clear units.

Unary ops like np.isfinite, np.isnan should return unitless patches.

-    out = operator(patch.data, *args, **kwargs)
-    return patch.new(data=out)
+    out = operator(patch.data, *args, **kwargs)
+    attrs = patch.attrs
+    if hasattr(out, "dtype") and np.issubdtype(np.asarray(out).dtype, np.bool_):
+        attrs = attrs.update(data_units=None)
+    return patch.new(data=out, attrs=attrs)

---

179-186: Clear units for boolean results in non‑unit binary path.

Comparisons should yield unitless patches.

-    else:
-        new_data = _apply_op(patch.data, other, operator, reversed)
+    else:
+        new_data = _apply_op(patch.data, other, operator, reversed)
+        if hasattr(new_data, "dtype") and np.issubdtype(
+            np.asarray(new_data).dtype, np.bool_
+        ):
+            attrs = attrs.update(data_units=None)
 
     new = patch.new(data=new_data, coords=coords, attrs=attrs)

Want a quick test asserting (patch > 0).attrs.data_units is None and same for unary isfinite?

dascore/utils/array.py (1)

528-533: Silence Ruff ARG001 by prefixing unused self parameters.

These are required by NumPy’s protocol but unused here.

-def patch_array_ufunc(self, ufunc, method, *inputs, **kwargs):
+def patch_array_ufunc(_patch, ufunc, method, *inputs, **kwargs):
@@
-def patch_array_function(self, func, types, args, kwargs):
+def patch_array_function(_patch, func, types, args, kwargs):

Also applies to: 536-541

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

dascore/utils/array.py (2)

187-203: Fix union-type isinstance (runtime TypeError) and avoid shadowing built-in ‘reversed’.

isinstance(other, Quantity | Unit) raises TypeError; use a tuple. Also, don’t shadow built-in reversed().

-    reversed = False
+    is_reversed = False
     if other_is_patch and not patch_is_patch:
         patch, other = other, patch
-        reversed = True
+        is_reversed = True
@@
-    if isinstance(other, Quantity | Unit):
-        new_data, attrs = _apply_op_units(patch, other, operator, attrs, reversed)
+    if isinstance(other, (Quantity, Unit)):
+        new_data, attrs = _apply_op_units(
+            patch, other, operator, attrs, reversed=is_reversed
+        )
     else:
-        new_data = _apply_op(patch.data, other, operator, reversed)
+        new_data = _apply_op(patch.data, other, operator, reversed=is_reversed)

---

376-379: Fix invalid isinstance check using str | bytes.

PEP 604 union cannot be used in isinstance. Use a tuple.

-        if isinstance(axis, Iterable) and not isinstance(axis, str | bytes):
+        if isinstance(axis, Iterable) and not isinstance(axis, (str, bytes)):
             axes = tuple(int(a) for a in axis)
         else:
             axes = (int(axis),)

dascore/utils/array.py (2)

536-541: Silence linter: unused self (keeps protocol intact).

Rename to _self to satisfy Ruff without behavior change.

-def patch_array_ufunc(self, ufunc, method, *inputs, **kwargs):
+def patch_array_ufunc(_self, ufunc, method, *inputs, **kwargs):

---

544-561: Silence linter: unused self (keeps protocol intact).

Same as above for array_function helper.

-def patch_array_function(self, func, types, args, kwargs):
+def patch_array_function(_self, func, types, args, kwargs):

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

dascore/utils/array.py (4)

52-57: Fix boolean-unit clearing helper (wrong dtype source, likely bad API).

This never clears units because Patch has no dtype; also update_attrs may not exist on Patch. Use data.dtype and Patch.new(attrs=attrs.update(...)).

-def _clear_units_if_bool_dtype(patch):
-    """Clear the units on the patch if it is a boolean."""
-    dtype = getattr(patch, "dtype", None)
-    if dtype is not None and np.issubdtype(dtype, np.bool_):
-        return patch.update_attrs(data_units=None)
-    return patch
+def _clear_units_if_bool_dtype(patch):
+    """Clear units when the patch's data dtype is boolean."""
+    dtype = np.asarray(patch.data).dtype
+    if np.issubdtype(dtype, np.bool_):
+        return patch.new(attrs=patch.attrs.update(data_units=None))
+    return patch

#!/bin/bash
# Verify whether Patch exposes update_attrs anywhere (it likely doesn't).
rg -nP 'def\s+update_attrs\s*\(' -C2
# Sanity: find any other dtype reads from Patch (should read from .data)
rg -nP '\.dtype\b' -C2

---

184-186: Use explicit exception instead of assert (asserts can be stripped).

Enforce API contract even under -O.

-    assert patch_count >= 1, "apply_ufunc requires at least one Patch operand."
+    if patch_count < 1:
+        raise ParameterError("apply_ufunc requires at least one Patch operand.")

---

198-203: Critical: invalid isinstance union and missing boolean-unit clearing (binary path).

isinstance(x, Quantity | Unit) raises TypeError. Also comparisons produce bools; clear units when result is boolean.

-    if isinstance(other, Quantity | Unit):
+    if isinstance(other, (Quantity, Unit)):
         new_data, attrs = _apply_op_units(patch, other, operator, attrs, reversed)
     else:
         new_data = _apply_op(patch.data, other, operator, reversed)
+        # Boolean outputs must be unitless.
+        if hasattr(new_data, "dtype") and np.issubdtype(np.asarray(new_data).dtype, np.bool_):
+            attrs = attrs.update(data_units=None)
     new = patch.new(data=new_data, coords=coords, attrs=attrs)
     return new

---

376-379: Critical: invalid isinstance with str | bytes.

isinstance(..., str | bytes) is invalid; use a tuple.

-        if isinstance(axis, Iterable) and not isinstance(axis, str | bytes):
+        if isinstance(axis, Iterable) and not isinstance(axis, (str, bytes)):
             axes = tuple(int(a) for a in axis)
         else:
             axes = (int(axis),)

dascore/utils/array.py (5)

447-456: Raise ParameterError instead of assert when no patches are found.

Improves robustness and error messaging.

-    assert len(patches), "No patches found in apply_array_func"
+    if not patches:
+        raise ParameterError("No patches found in apply_array_func")

---

536-542: Silence Ruff ARG001: rename unused self to _.

Signature compatibility is preserved; reduces noise.

-def patch_array_ufunc(self, ufunc, method, *inputs, **kwargs):
+def patch_array_ufunc(_, ufunc, method, *inputs, **kwargs):

---

544-561: Silence Ruff ARG001: rename unused self to _.

Same rationale as above.

-def patch_array_function(self, func, types, args, kwargs):
+def patch_array_function(_, func, types, args, kwargs):

---

152-167: Avoid shadowing built-in reversed.

Minor readability nit; rename local to is_reversed.

-def _apply_op(array1, array2, operator, reversed=False):
+def _apply_op(array1, array2, operator, is_reversed=False):
     """Simply apply the operator, account for reversal."""
-    if reversed:
+    if is_reversed:
         array1, array2 = array2, array1
     return operator(array1, array2, *args, **kwargs)
@@
-def _apply_op_units(patch, other, operator, attrs, reversed=False):
+def _apply_op_units(patch, other, operator, attrs, is_reversed=False):
@@
-    try:
-        new_data_w_units = _apply_op(data, other, operator, reversed=reversed)
+    try:
+        new_data_w_units = _apply_op(data, other, operator, is_reversed=is_reversed)
@@
-    reversed = False
+    is_reversed = False
     if other_is_patch and not patch_is_patch:
         patch, other = other, patch
-        reversed = True
+        is_reversed = True
@@
-        new_data, attrs = _apply_op_units(patch, other, operator, attrs, reversed)
+        new_data, attrs = _apply_op_units(patch, other, operator, attrs, is_reversed)
     else:
-        new_data = _apply_op(patch.data, other, operator, reversed)
+        new_data = _apply_op(patch.data, other, operator, is_reversed)

Also applies to: 187-191

---

427-444: Prefer explicit error over assert for unknown ufunc method.

Edge-case, but better UX if a non-reduce/accumulate method sneaks in.

-        name = getattr(func, "__name__", None)
-        assert name in ufunc_method_name
-        sig = inspect.signature(ufunc_method_name[name])
+        name = getattr(func, "__name__", None)
+        if name not in ufunc_method_name:
+            raise ParameterError(f"Unsupported ufunc method '{name}'.")
+        sig = inspect.signature(ufunc_method_name[name])

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro