Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 99 additions & 0 deletions dascore/core/coords.py
Original file line number Diff line number Diff line change
Expand Up @@ -430,10 +430,109 @@ def __str__(self):

__repr__ = __str__

__array_priority__ = 1000.0

def __array__(self, dtype=None, copy=False):
"""Numpy method for getting array data with `np.array(coord)`."""
return self.data

def _get_coord_output(self, data, units=None):
"""Return output from operations as a coordinate when possible."""
if isinstance(data, BaseCoord):
return data
if hasattr(data, "magnitude") and hasattr(data, "units"):
return get_coord(data=data.magnitude, units=data.units)
return get_coord(data=data, units=units)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Return scalar NumPy results instead of wrapping as coords

The output helper always calls get_coord for non-BaseCoord values, so scalar-returning NumPy operations now produce invalid coordinates rather than numbers. In practice, reductions like np.sum(coord) and scalar linear algebra calls such as np.dot(coord, coord) are routed through this path and become CoordPartial objects with NaN-filled data instead of the expected scalar value, which corrupts numerical results.

Useful? React with 👍 / 👎.


def _binary_coord_op(self, operator, other, reversed=False):
"""Apply a binary operator and return a new coordinate."""
other_data = other.data if isinstance(other, BaseCoord) else other
# Addition/subtraction treat scalars as values in current units.
if hasattr(other_data, "units") and operator in (np.add, np.subtract):
other_data = convert_units(
other_data.magnitude,
self.units,
other_data.units,
)
lhs, rhs = (other_data, self.data) if reversed else (self.data, other_data)
out = operator(lhs, rhs)
Comment on lines +457 to +458

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep self units in non-additive quantity arithmetic

Binary ops compute using self.data (unitless ndarray), so when other is a quantity in multiply/divide/power/modulo paths, NumPy derives units from other alone and loses the coordinate’s units. A concrete case is get_coord(..., units='m') * (3 * s), which currently yields units of s instead of m*s. This introduces incorrect dimensional metadata for quantity arithmetic.

Useful? React with 👍 / 👎.

units = (
self.units if not np.issubdtype(np.asarray(out).dtype, np.bool_) else None
)
return self._get_coord_output(out, units=units)
Comment on lines +447 to +462
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Unit handling for coord + coord with differing units is not addressed.

When other is a BaseCoord, other_data = other.data strips the unit information. The unit-conversion branch (line 451) only triggers for objects with a .units attribute on the data itself (e.g., pint Quantities), not for BaseCoord operands whose .data is a plain ndarray. This means coord_m + coord_ft silently adds raw arrays without converting, producing incorrect values.

Consider adding a branch like:

Proposed fix
     def _binary_coord_op(self, operator, other, reversed=False):
         """Apply a binary operator and return a new coordinate."""
         other_data = other.data if isinstance(other, BaseCoord) else other
+        # Convert coord-to-coord units for add/sub
+        if (
+            isinstance(other, BaseCoord)
+            and other.units is not None
+            and self.units is not None
+            and operator in (np.add, np.subtract)
+        ):
+            other_data = convert_units(other_data, self.units, other.units)
         # Addition/subtraction treat scalars as values in current units.
-        if hasattr(other_data, "units") and operator in (np.add, np.subtract):
+        elif hasattr(other_data, "units") and operator in (np.add, np.subtract):
🤖 Prompt for AI Agents
In `@dascore/core/coords.py` around lines 447 - 462, _binary_coord_op currently
strips unit info by setting other_data = other.data when other is a BaseCoord;
add logic to detect when other is a BaseCoord and operator is np.add or
np.subtract and then convert other.data from other.units into self.units via
convert_units (use other.units and other.data.magnitude or plain array as
appropriate), handling the reversed flag consistently (i.e., convert rhs when
reversed or lhs otherwise), and then call _get_coord_output with the proper
units; reference symbols: _binary_coord_op, BaseCoord, other.data, other.units,
convert_units, _get_coord_output, operator, reversed.


def __add__(self, other):
return self._binary_coord_op(np.add, other)

def __sub__(self, other):
return self._binary_coord_op(np.subtract, other)

def __mul__(self, other):
return self._binary_coord_op(np.multiply, other)

def __truediv__(self, other):
return self._binary_coord_op(np.divide, other)

def __floordiv__(self, other):
return self._binary_coord_op(np.floor_divide, other)

def __pow__(self, other):
return self._binary_coord_op(np.power, other)

def __mod__(self, other):
return self._binary_coord_op(np.mod, other)

__radd__ = __add__

def __rsub__(self, other):
return self._binary_coord_op(np.subtract, other, reversed=True)

__rmul__ = __mul__

def __rtruediv__(self, other):
return self._binary_coord_op(np.divide, other, reversed=True)

def __rfloordiv__(self, other):
return self._binary_coord_op(np.floor_divide, other, reversed=True)

def __rpow__(self, other):
return self._binary_coord_op(np.power, other, reversed=True)

def __rmod__(self, other):
return self._binary_coord_op(np.mod, other, reversed=True)

def __array_ufunc__(self, ufunc, method, *inputs, **kwargs):
"""Support numpy ufunc operations and return coordinate outputs."""
method_func = ufunc if method == "__call__" else getattr(ufunc, method)
converted = [x.data if isinstance(x, BaseCoord) else x for x in inputs]
out = method_func(*converted, **kwargs)
units = (
self.units if not np.issubdtype(np.asarray(out).dtype, np.bool_) else None
)
return self._get_coord_output(out, units=units)
Comment on lines 504 to 512
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix line-length violation flagged by CI (line 503).

Same E501 pattern as above.

🔧 Proposed fix
-        units = self.units if not np.issubdtype(np.asarray(out).dtype, np.bool_) else None
+        is_bool = np.issubdtype(np.asarray(out).dtype, np.bool_)
+        units = self.units if not is_bool else None
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def __array_ufunc__(self, ufunc, method, *inputs, **kwargs):
"""Support numpy ufunc operations and return coordinate outputs."""
method_func = ufunc if method == "__call__" else getattr(ufunc, method)
converted = [x.data if isinstance(x, BaseCoord) else x for x in inputs]
out = method_func(*converted, **kwargs)
units = self.units if not np.issubdtype(np.asarray(out).dtype, np.bool_) else None
return self._get_coord_output(out, units=units)
def __array_ufunc__(self, ufunc, method, *inputs, **kwargs):
"""Support numpy ufunc operations and return coordinate outputs."""
method_func = ufunc if method == "__call__" else getattr(ufunc, method)
converted = [x.data if isinstance(x, BaseCoord) else x for x in inputs]
out = method_func(*converted, **kwargs)
is_bool = np.issubdtype(np.asarray(out).dtype, np.bool_)
units = self.units if not is_bool else None
return self._get_coord_output(out, units=units)
🧰 Tools
🪛 GitHub Actions: LintCode

[error] 503-503: E501 Line too long (90 > 88).

🤖 Prompt for AI Agents
In `@dascore/core/coords.py` around lines 498 - 504, The long single-line ternary
that sets units in __array_ufunc__ exceeds line-length; split the expression
into clear intermediate steps: convert out to an array (e.g., out_arr =
np.asarray(out)), compute a boolean flag using np.issubdtype(out_arr.dtype,
np.bool_), then set units = self.units if not that flag else None, and finally
call self._get_coord_output(out, units=units). Reference symbols:
__array_ufunc__, BaseCoord, _get_coord_output, np.asarray, np.issubdtype.


def __array_function__(self, func, types, args, kwargs):
"""Support NumPy array-function protocol for coordinates."""
if not any(issubclass(t, BaseCoord) for t in types):
return NotImplemented

def _convert(obj):
if isinstance(obj, BaseCoord):
return obj.data
Comment on lines +520 to +521

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Validate units before stripping BaseCoord args in array API

The array-function conversion replaces each BaseCoord input with raw .data and drops its units, then the result is labeled with self.units. For multi-input calls this can silently relabel incompatible values: for example np.concatenate((coord_m, coord_s)) appends second-based values unchanged but returns a coordinate tagged as meters. This is a data-integrity bug because mixed-unit inputs are neither converted nor rejected.

Useful? React with 👍 / 👎.

if isinstance(obj, tuple):
return tuple(_convert(x) for x in obj)
if isinstance(obj, list):
return [_convert(x) for x in obj]
if isinstance(obj, dict):
return {k: _convert(v) for k, v in obj.items()}
return obj

out = func(*_convert(args), **_convert(kwargs))
units = (
self.units if not np.issubdtype(np.asarray(out).dtype, np.bool_) else None
)
return self._get_coord_output(out, units=units)
Comment on lines 514 to 534
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, locate and read the file
git ls-files | grep -E "(coords\.py|core/coords\.py)"

Repository: DASDAE/dascore

Length of output: 170


🏁 Script executed:

# Read the specific lines mentioned in the review
cat -n dascore/core/coords.py | sed -n '500,530p'

Repository: DASDAE/dascore

Length of output: 1568


🏁 Script executed:

# Check the exact character count of line 523
sed -n '523p' dascore/core/coords.py | wc -c

Repository: DASDAE/dascore

Length of output: 60


🏁 Script executed:

python3 << 'EOF'
import numpy as np

# Test case 1: np.asarray on tuple of arrays
print("Test 1: np.asarray on tuple of arrays")
arr1 = np.array([1, 2, 3])
arr2 = np.array([4, 5, 6])
result_tuple = (arr1, arr2)
asarray_result = np.asarray(result_tuple)
print(f"  Input: tuple of 2 arrays")
print(f"  Result dtype: {asarray_result.dtype}")
print(f"  Result shape: {asarray_result.shape}")

# Test case 2: np.asarray on scalar
print("\nTest 2: np.asarray on scalar")
scalar = 5
asarray_scalar = np.asarray(scalar)
print(f"  Input: scalar 5")
print(f"  Result dtype: {asarray_scalar.dtype}")

# Test case 3: np.issubdtype with object dtype
print("\nTest 3: np.issubdtype on object dtype")
obj_arr = np.asarray([(1, 2), (3, 4)])
print(f"  Object array dtype: {obj_arr.dtype}")
is_bool = np.issubdtype(obj_arr.dtype, np.bool_)
print(f"  np.issubdtype(object, np.bool_) = {is_bool}")

# Test case 4: Real numpy function returning tuple
print("\nTest 4: np.unique with return_index=True (returns tuple)")
data = np.array([1, 2, 1, 3, 2])
result = np.unique(data, return_index=True)
print(f"  np.unique output type: {type(result)}")
print(f"  np.unique output: {result}")
asarray_unique = np.asarray(result)
print(f"  np.asarray(result).dtype: {asarray_unique.dtype}")
is_bool_unique = np.issubdtype(asarray_unique.dtype, np.bool_)
print(f"  np.issubdtype on this object dtype: {is_bool_unique}")

# Test case 5: Boolean array as output
print("\nTest 5: np.isnan returns boolean array")
data = np.array([1.0, np.nan, 3.0])
result = np.isnan(data)
asarray_bool = np.asarray(result)
print(f"  np.isnan output dtype: {asarray_bool.dtype}")
is_bool_result = np.issubdtype(asarray_bool.dtype, np.bool_)
print(f"  np.issubdtype(bool_, np.bool_) = {is_bool_result}")

EOF

Repository: DASDAE/dascore

Length of output: 175


🏁 Script executed:

# Find the _get_coord_output method to understand what it expects
rg -A 10 "def _get_coord_output" dascore/core/coords.py

Repository: DASDAE/dascore

Length of output: 638


🏁 Script executed:

pip install numpy -q && python3 << 'EOF'
import numpy as np

# Test case 1: np.asarray on tuple of arrays
print("Test 1: np.asarray on tuple of arrays")
arr1 = np.array([1, 2, 3])
arr2 = np.array([4, 5, 6])
result_tuple = (arr1, arr2)
asarray_result = np.asarray(result_tuple)
print(f"  Input: tuple of 2 arrays")
print(f"  Result dtype: {asarray_result.dtype}")
print(f"  Result shape: {asarray_result.shape}")

# Test case 2: np.asarray on scalar
print("\nTest 2: np.asarray on scalar")
scalar = 5
asarray_scalar = np.asarray(scalar)
print(f"  Input: scalar 5")
print(f"  Result dtype: {asarray_scalar.dtype}")

# Test case 3: np.issubdtype with object dtype
print("\nTest 3: np.issubdtype on object dtype")
obj_arr = np.asarray([(1, 2), (3, 4)])
print(f"  Object array dtype: {obj_arr.dtype}")
is_bool = np.issubdtype(obj_arr.dtype, np.bool_)
print(f"  np.issubdtype(object, np.bool_) = {is_bool}")

# Test case 4: Real numpy function returning tuple
print("\nTest 4: np.unique with return_index=True (returns tuple)")
data = np.array([1, 2, 1, 3, 2])
result = np.unique(data, return_index=True)
print(f"  np.unique output type: {type(result)}")
print(f"  np.unique output: {result}")
asarray_unique = np.asarray(result)
print(f"  np.asarray(result).dtype: {asarray_unique.dtype}")
is_bool_unique = np.issubdtype(asarray_unique.dtype, np.bool_)
print(f"  np.issubdtype on this object dtype: {is_bool_unique}")

# Test case 5: Boolean array as output
print("\nTest 5: np.isnan returns boolean array")
data = np.array([1.0, np.nan, 3.0])
result = np.isnan(data)
asarray_bool = np.asarray(result)
print(f"  np.isnan output dtype: {asarray_bool.dtype}")
is_bool_result = np.issubdtype(asarray_bool.dtype, np.bool_)
print(f"  np.issubdtype(bool_, np.bool_) = {is_bool_result}")

EOF

Repository: DASDAE/dascore

Length of output: 935


🏁 Script executed:

rg -B 2 -A 20 "^def get_coord" dascore/core/coords.py | head -50

Repository: DASDAE/dascore

Length of output: 596


Fix line-length violation on line 523 (exceeds 88-character limit).

Line 523 is 91 characters and must be shortened. The proposed refactoring improves both compliance and readability:

🔧 Proposed fix
-        units = self.units if not np.issubdtype(np.asarray(out).dtype, np.bool_) else None
+        is_bool = np.issubdtype(np.asarray(out).dtype, np.bool_)
+        units = self.units if not is_bool else None
🧰 Tools
🪛 GitHub Actions: LintCode

[error] 523-523: E501 Line too long (90 > 88).

🤖 Prompt for AI Agents
In `@dascore/core/coords.py` around lines 506 - 524, The line in
__array_function__ that computes units is over the length limit; refactor the
boolean dtype check into a short intermediate variable and then set units using
that variable to keep lines under 88 chars. Specifically, in __array_function__
(use symbols __array_function__, _convert, _get_coord_output) extract
np.issubdtype(np.asarray(out).dtype, np.bool_) into a named variable (e.g.,
is_bool) on its own line and then assign units = self.units if not is_bool else
None before returning self._get_coord_output(out, units=units).


@cached_method
def min(self):
"""Return min value."""
Expand Down
83 changes: 83 additions & 0 deletions tests/test_core/test_coords.py
Original file line number Diff line number Diff line change
Expand Up @@ -1895,6 +1895,89 @@ def test_not_implemented_in_baseclass(self, evenly_sampled_coord):
BaseCoord.change_length(coord, 10)




class TestCoordinateArithmetic:
"""Tests for coordinate arithmetic behavior (issue #566)."""

@pytest.mark.parametrize(
"func, expected, other",
[
(lambda coord, val: coord + val, lambda vals, val: vals + val, 2),
(lambda coord, val: coord - val, lambda vals, val: vals - val, 2),
(lambda coord, val: coord * val, lambda vals, val: vals * val, 2),
(lambda coord, val: coord / val, lambda vals, val: vals / val, 2),
(lambda coord, val: coord // val, lambda vals, val: vals // val, 2),
(lambda coord, val: coord**val, lambda vals, val: vals**val, 2),
(lambda coord, val: coord % val, lambda vals, val: vals % val, 3),
(lambda coord, val: val + coord, lambda vals, val: val + vals, 2),
(lambda coord, val: val - coord, lambda vals, val: val - vals, 10),
(lambda coord, val: val * coord, lambda vals, val: val * vals, 2),
(lambda coord, val: val / coord, lambda vals, val: val / vals, 10),
(lambda coord, val: val // coord, lambda vals, val: val // vals, 10),
(lambda coord, val: val**coord, lambda vals, val: val**vals, 2),
(lambda coord, val: val % coord, lambda vals, val: val % vals, 10),
],
)
def test_all_dunder_binary_ops(self, func, expected, other):
"""Ensure all new arithmetic dunder paths return coordinates."""
coord = get_coord(data=[2, 4, 8], units="m")

out = func(coord, other)
assert isinstance(out, BaseCoord)
assert out.units == coord.units
np.testing.assert_allclose(out.values, expected(coord.values, other))

def test_numpy_ufunc_call_returns_coord(self):
"""Ensure numpy ufunc __call__ dispatch returns coordinates."""
coord = get_coord(data=[1, 4, 9], units="m")
out = np.sqrt(coord)

assert isinstance(out, BaseCoord)
assert out.units == coord.units
np.testing.assert_allclose(out.values, np.array([1.0, 2.0, 3.0]))

def test_numpy_ufunc_accumulate_returns_coord(self):
"""Ensure numpy ufunc method dispatch (e.g. accumulate) returns coordinates."""
coord = get_coord(data=[1, 2, 3], units="m")
out = np.add.accumulate(coord)

assert isinstance(out, BaseCoord)
assert out.units == coord.units
np.testing.assert_array_equal(out.values, np.array([1, 3, 6]))

def test_numpy_array_function_returns_coord(self):
"""Ensure numpy array functions return coordinates where possible."""
coord = get_coord(data=[3, 4], units="m")
out = np.linalg.norm(coord)

assert isinstance(out, BaseCoord)
assert out.units == coord.units
np.testing.assert_allclose(out.values, np.array([5.0]))

def test_tuple_list_dict_conversions(self):
"""Ensure tuple/list/dict conversion paths are exercised."""
coord1 = get_coord(data=[1, 2], units="m")
coord2 = get_coord(data=[3, 4], units="m")

out_tuple = np.concatenate((coord1, coord2))
assert isinstance(out_tuple, BaseCoord)
np.testing.assert_array_equal(out_tuple.values, np.array([1, 2, 3, 4]))

out_list = np.concatenate([coord1, coord2])
assert isinstance(out_list, BaseCoord)
np.testing.assert_array_equal(out_list.values, np.array([1, 2, 3, 4]))

out_kwargs = np.mean(a=coord1)
assert isinstance(out_kwargs, BaseCoord)
np.testing.assert_allclose(out_kwargs.values, np.array([1.5]))

Comment on lines +1958 to +1974
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Good coverage of conversion paths; missing blank lines before next class.

The test name test_tuple_list_dict_conversions clearly reflects the intent (per PR objective). PEP 8 expects two blank lines between top-level class definitions, but there appear to be none between lines 1950 and class TestIssues at line 1952.

🔧 Proposed fix
         out_kwargs = np.mean(a=coord1)
         assert isinstance(out_kwargs, BaseCoord)
         np.testing.assert_allclose(out_kwargs.values, np.array([1.5]))
+
+
 class TestIssues:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def test_tuple_list_dict_conversions(self):
"""Ensure tuple/list/dict conversion paths are exercised."""
coord1 = get_coord(data=[1, 2], units="m")
coord2 = get_coord(data=[3, 4], units="m")
out_tuple = np.concatenate((coord1, coord2))
assert isinstance(out_tuple, BaseCoord)
np.testing.assert_array_equal(out_tuple.values, np.array([1, 2, 3, 4]))
out_list = np.concatenate([coord1, coord2])
assert isinstance(out_list, BaseCoord)
np.testing.assert_array_equal(out_list.values, np.array([1, 2, 3, 4]))
out_kwargs = np.mean(a=coord1)
assert isinstance(out_kwargs, BaseCoord)
np.testing.assert_allclose(out_kwargs.values, np.array([1.5]))
def test_tuple_list_dict_conversions(self):
"""Ensure tuple/list/dict conversion paths are exercised."""
coord1 = get_coord(data=[1, 2], units="m")
coord2 = get_coord(data=[3, 4], units="m")
out_tuple = np.concatenate((coord1, coord2))
assert isinstance(out_tuple, BaseCoord)
np.testing.assert_array_equal(out_tuple.values, np.array([1, 2, 3, 4]))
out_list = np.concatenate([coord1, coord2])
assert isinstance(out_list, BaseCoord)
np.testing.assert_array_equal(out_list.values, np.array([1, 2, 3, 4]))
out_kwargs = np.mean(a=coord1)
assert isinstance(out_kwargs, BaseCoord)
np.testing.assert_allclose(out_kwargs.values, np.array([1.5]))
🤖 Prompt for AI Agents
In `@tests/test_core/test_coords.py` around lines 1935 - 1951, There are not two
blank lines separating the top-level test function
test_tuple_list_dict_conversions and the next top-level class TestIssues; add
the required blank lines per PEP8 by inserting one additional blank line so
there are two blank lines between the end of test_tuple_list_dict_conversions
and the declaration of class TestIssues (edit the area around the end of
test_tuple_list_dict_conversions and the start of class TestIssues to ensure two
blank lines).

def test_array_function_returns_not_implemented_for_non_coord_types(self):
"""Ensure non-coordinate array_function dispatch returns NotImplemented."""
coord = get_coord(data=[1, 2, 3], units="m")
out = coord.__array_function__(np.mean, (np.ndarray,), (coord,), {})
assert out is NotImplemented

class TestIssues:
"""Tests for special issues related to coords."""

Expand Down
Loading