Fix 566: coordinate arithmetic - #608
Conversation
📝 WalkthroughWalkthroughBaseCoord now supports Python operators and NumPy protocols with unit-aware operand conversion, immutable operations, coordinate result wrapping, boolean comparisons, and time-like reductions. Tests, documentation, unit exception exports, and datetime-coordinate fixtures were updated. ChangesCoordinate NumPy interoperability
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb527a1220
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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) |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed. Array operations no longer force everything into a coordinate: _wrap_array_op_output returns scalars as scalars (as a Quantity when the coord has units), boolean arrays as arrays (they are masks), and only wraps genuine arrays into coordinates. np.mean(coord) now returns e.g. 4.67 m.
| if isinstance(obj, BaseCoord): | ||
| return obj.data |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Fixed. __array_function__ now converts each coord to a pint Quantity (via _to_operand) instead of stripping units, so pint validates/propagates the units and the result is no longer blindly labeled with self.units. Incompatible units raise a UnitError.
| lhs, rhs = (other_data, self.data) if reversed else (self.data, other_data) | ||
| out = operator(lhs, rhs) |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Fixed. Operands are now converted to pint quantities and pint does the unit algebra, so coord_m * coord_m -> m ** 2, coord_m / coord_m -> dimensionless, coord_m ** 2 -> m ** 2, etc. The old "always keep self.units" behavior is gone.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@dascore/core/coords.py`:
- Around line 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.
- Around line 447-456: The long lines in _binary_coord_op come from the compound
conditional and the inline conditional determining units; fix by splitting into
intermediate variables: extract the operator set (e.g., ops = (np.add,
np.subtract)) and use if hasattr(other_data, "units") and operator in ops to
shorten that check, and compute a boolean like is_bool =
np.issubdtype(np.asarray(out).dtype, np.bool_) then set units = self.units if
not is_bool else None; update references to other_data, operator, out, and units
accordingly in _binary_coord_op.
- Around line 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).
In `@tests/test_core/test_coords.py`:
- Around line 1917-1924: The test asserts units are preserved for np.sqrt which
is dimensionally wrong; update the behavior in BaseCoord.__array_ufunc__ to
propagate unit exponents for root/power ufuncs (detect np.sqrt / ufunc.__name__
== "sqrt" or ufunc == np.sqrt and compute new_units = self.units ** 0.5, then
set the result's units to new_units) and adjust the test
test_numpy_ufunc_returns_coord to expect units "m**0.5" (or the units object's
equivalent) and values [1.0,2.0,3.0]; reference BaseCoord.__array_ufunc__ and
the test function test_numpy_ufunc_returns_coord when making changes.
- Around line 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).
🧹 Nitpick comments (1)
dascore/core/coords.py (1)
455-455: Duplicated "is result boolean?" logic across three methods — consider extracting a helper.The pattern
self.units if not np.issubdtype(np.asarray(out).dtype, np.bool_) else Noneappears identically in_binary_coord_op,__array_ufunc__, and__array_function__. Extracting a small helper (e.g.,_units_for_output(self, out)) would reduce duplication and make the line-length fix apply in one place.Also applies to: 503-503, 523-523
| 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 | ||
| 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) |
There was a problem hiding this comment.
🧩 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 -cRepository: 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}")
EOFRepository: 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.pyRepository: 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}")
EOFRepository: DASDAE/dascore
Length of output: 935
🏁 Script executed:
rg -B 2 -A 20 "^def get_coord" dascore/core/coords.py | head -50Repository: 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).
There was a problem hiding this comment.
Agreed, the duplication is gone. All three protocols (__array_ufunc__, __array_function__, and the operator dunders, which now just call ufuncs) share _to_operand and _wrap_array_op_output.
There was a problem hiding this comment.
@d-chambers, agreed—thanks for clarifying. The current structure centralizes operand conversion and result wrapping cleanly, while the dunders appropriately delegate through the ufunc path. 🐇
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@dascore/core/coords.py`:
- Around line 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.
🧹 Nitpick comments (1)
dascore/core/coords.py (1)
504-512: Multi-output ufuncs (e.g.,np.modf,np.divmod) will produce unexpected results.When a ufunc returns a tuple of arrays,
np.asarray(out)creates a 2D array and_get_coord_outputwraps it into a single coordinate rather than returning a tuple of coordinates. This is unlikely to be hit in normal usage now, but worth a guard:if isinstance(out, tuple): return tuple(self._get_coord_output(o, units=units) for o in out)Not blocking since the PR scope is basic arithmetic, but it would prevent confusing errors if users call ufuncs like
np.modf(coord).
Coordinates now implement the numpy ufunc and array function protocols by delegating unit handling to pint, so units reflect the operation performed (eg m * m -> m ** 2, sqrt(m) -> m ** 0.5) rather than simply being carried over from the left operand. Scalars are assumed to be in the coordinate's units for operations which require matching units. Reductions return scalars (quantities when units are set) and boolean results return arrays since neither is useful as a coordinate. Invalid unit combinations now raise UnitError and the out parameter raises ParameterError, matching the behavior of Patch operations.
159e9ad to
b469057
Compare
|
Rebased onto master and reworked the implementation so units track the operation rather than persisting from the left operand (see updated description). All review comments from the previous round should be addressed in the replies above. @codex review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #608 +/- ##
========================================
Coverage 99.93% 99.93%
========================================
Files 145 145
Lines 12838 12964 +126
========================================
+ Hits 12830 12956 +126
Misses 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b469057984
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not self._units_are_quantifiable: | ||
| # Time-like coords (and coords with no units) operate on raw | ||
| # values; pint knows nothing of datetime64/timedelta64. | ||
| return data |
There was a problem hiding this comment.
When the dispatching coordinate is unitless, this branch returns raw data for every operand, including coordinates and quantities that do have units. Consequently, unitless_coord * meter_coord silently produces a unitless coordinate, while reversing the operands produces one in meters; matched operations such as addition can also succeed after discarding an incompatible right-hand unit. Preserve unitful operands independently of self._units_are_quantifiable, limiting the raw-value exception to time-like data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed. The units used for unit-less operands now come from _get_op_units, which prefers self.units and otherwise takes the units of the first operand that has them, so unitless_coord * meter_coord and the reverse both give meters. The raw-value exception is now keyed only on time-like dtypes. Test: test_units_of_other_operands_used.
| if method != "__call__": | ||
| # Pint doesn't implement reduce/accumulate/outer/at, so those | ||
| # are applied to raw values and units handled here. | ||
| operands = [getattr(x, "magnitude", x) for x in operands] |
There was a problem hiding this comment.
For outer, reduce, and accumulate, stripping each quantity to its magnitude bypasses both unit conversion and unit algebra. For example, np.add.outer([2] m, [100] cm) adds the raw values and returns 102 m rather than 3 m, because the result is subsequently labeled with self.units; similarly, np.multiply.reduce loses the accumulated power of the unit. These methods need operation-specific unit handling rather than applying the ufunc directly to unrelated magnitudes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. For non-__call__ methods the operands are now converted to the shared units before their magnitudes are taken (_to_magnitude), so np.add.outer(coord_m, coord_cm) gives 3 m — test test_ufunc_method_converts_units. For methods where the units genuinely change (np.multiply.reduce) there is no correct answer to compute here, so those now raise a UnitError telling the user to operate on the values instead, rather than silently mislabeling the result.
| with _unit_error_context(ufunc, self.unit_str): | ||
| out = getattr(ufunc, method)(*operands, **kwargs) |
There was a problem hiding this comment.
at ufunc method
Although out= is rejected above to enforce immutability, np.add.at(unitless_coord, [0], 1) reaches this call with the coordinate replaced by its internal writable array and mutates it in place. This can also leave cached values such as min() and max() stale. Reject the at method (or operate on a copy and return a new coordinate) before invoking the ufunc.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, at does reach the internal array. It is now rejected alongside out= with a ParameterError, covered by test_mutating_operations_raise.
| args = _map_nested(self._to_operand, args) | ||
| kwargs = _map_nested(self._to_operand, kwargs) | ||
| with _unit_error_context(func, self.unit_str): | ||
| out = func(*args, **kwargs) |
There was a problem hiding this comment.
For a unitless coordinate, _map_nested(self._to_operand, args) exposes the coordinate's internal array directly to array functions. A destination-style call such as np.copyto(coord, 0) therefore mutates the supposedly immutable coordinate and can invalidate cached metadata, despite the explicit immutability protection in __array_ufunc__. Mutating array functions and destination arguments need to be rejected or supplied copies.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. __array_function__ rejects a set of known destination-style functions (copyto, place, put, put_along_axis, putmask, fill_diagonal) and any call passing out=, both with a ParameterError.
There was a problem hiding this comment.
Correction to my earlier reply: I checked this one empirically and the premise is wrong. Coord arrays are read-only (dascore.compat.array clears the writeable flag), and numpy honors that for np.copyto, __setitem__, and out= — all three raise ValueError: assignment destination is read-only. So there was no mutation hole here; the guard only replaces that ValueError with a ParameterError explaining why.
The at case (the sibling comment) is different and genuinely needed: np.add.at ignores the read-only flag entirely on numpy 2.4.6, so it really did mutate the coordinate. Comments in the code now say which is which.
There was a problem hiding this comment.
Following up: rather than keep a guard that duplicates protection numpy already provides, the custom rejection is removed. np.copyto, np.put, np.putmask, np.place, and out= all raise numpy's own ValueError: assignment destination is read-only against a coordinate, which test_operations_writing_to_coord_raise now pins. Only ufunc.at keeps an explicit ParameterError, because that is the one case where numpy ignores the flag.
| if isinstance(out, Quantity): | ||
| out, units = out.magnitude, out.units | ||
| units = None if units.dimensionless else units |
There was a problem hiding this comment.
This drops every dimensionless Pint unit without converting its magnitude to an unscaled dimensionless value. For example, dividing a coordinate containing 50 percent by 2 produces 25 percent; this code removes the percent unit but leaves the magnitude as 25, yielding a unitless value that is 100 times the physical value of 0.25. Compatible ratios such as meters divided by centimeters can be corrupted similarly, so convert to base/dimensionless units before clearing units.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — coord_m / coord_cm came out 100x off. _wrap_array_op_output now converts with .to("dimensionless") before clearing the units, so the scale is applied to the magnitude. Test: test_scaled_dimensionless_output.
| args = _map_nested(self._to_operand, args) | ||
| kwargs = _map_nested(self._to_operand, kwargs) | ||
| with _unit_error_context(func, self.unit_str): | ||
| out = func(*args, **kwargs) |
There was a problem hiding this comment.
For a datetime coordinate, _to_operand supplies the raw datetime64 array and this invokes functions such as np.mean directly, but NumPy cannot sum absolute datetimes to calculate their mean. The existing reduce_coord path deliberately uses _reduce_time_like for this case, so the newly advertised np.mean(datetime_coord) fails even though the same operation works through the coordinate reduction API. Apply the existing time-aware reduction logic when dispatching these NumPy reducers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed. Time-like coords are routed through the existing _reduce_time_like for the reducing numpy functions, so np.mean(time_coord) returns a datetime64. This also surfaced a related bug: a time-like scalar result had self.units multiplied onto it, which raised a UFuncTypeError. Test: test_time_coord_reductions.
| if isinstance(out, tuple): # Some ufuncs (eg np.divmod) return tuples. | ||
| return tuple(_wrap_array_op_output(x, units) for x in out) |
There was a problem hiding this comment.
Only tuple outputs are recursively wrapped, so array functions returning lists bypass coordinate construction entirely. In particular, np.array_split(coord, 2) returns a list of raw arrays for a unitless coordinate or Pint quantities for a unitful one, rather than the documented new coordinates, making the API's result type depend on the input units. Handle list outputs alongside tuples so each split result is wrapped consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed, lists are wrapped like tuples now, so np.array_split returns coordinates. Test: test_array_function_returning_list. (For a coord with units pint itself refuses array_split, which surfaces as a TypeError from numpy.)
| def __abs__(self): | ||
| return self._operate(np.absolute, self) |
There was a problem hiding this comment.
The matched-ufunc set includes greater, greater_equal, less, and less_equal, but the new Python operator block never forwards the corresponding ordering dunders. As a result, coord > 4 * get_quantity("m") raises TypeError instead of producing the same mask as np.greater(coord, ...), even though coordinates are documented as supporting Python operators and Patch already exposes these comparisons. Add __gt__, __ge__, __lt__, and __le__ forwarding to the registered ufuncs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Added __gt__, __ge__, __lt__, and __le__. __eq__/__ne__ are deliberately left alone: coords are pydantic models and their equality compares coordinates (used in align, spool equality, etc.), not values. There is a comment in the code noting this.
| try: | ||
| yield | ||
| except DimensionalityError as ex: |
There was a problem hiding this comment.
This context only translates Pint's DimensionalityError, so invalid operations involving offset units can leak a different Pint exception through the public coordinate API. For example, adding two coordinates in degrees Celsius raises Pint's OffsetUnitCalculusError rather than the documented DASCore UnitError. Catch the applicable Pint unit-operation exception family here so callers receive the promised stable exception type.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Changed to catch pint's PintError base class, so any pint failure surfaces as a UnitError. Worth noting the specific example does not actually raise: pint only rejects offset units in Quantity.__add__, while np.add on two degC quantities succeeds, and coord operations always go through the ufunc. The broader catch makes the promise hold regardless.
- Units from any operand are used, so unitless_coord * meter_coord no longer discards the meters (and matches the reversed order). - Ufunc methods (reduce/accumulate/outer) convert operands to the shared units rather than stripping magnitudes; methods whose units can't be determined (eg multiply.reduce) now raise UnitError instead of silently dropping units. - Reject np.add.at and mutating array functions (eg np.copyto) since they modify the coordinate's array in place. - Convert scaled dimensionless results (eg m / cm) before dropping units so values are not off by the scale factor. - Route time-like coords through _reduce_time_like so np.mean and friends work on datetime coords, and never attach units to a time-like scalar. - Wrap list outputs (eg np.array_split) like tuple outputs. - Add ordering dunders, keeping __eq__ as model equality. - Translate any pint error, not just DimensionalityError, to UnitError.
|
Pushed fixes for all nine points from the codex review; details are in the replies on each thread. The P1s were real: unitless coords discarded other operands' units, ufunc methods skipped unit conversion, @codex review |
coderabbitai
Bot
added
bug
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_core/test_coords.py (1)
2174-2174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit
strict=tozip().Ruff flags
B905here.outandexpectedare guaranteed equal length, sostrict=Truealso documents that invariant and matches the codebase convention.♻️ Proposed tweak
- for coord_out, array_out in zip(out, expected): + for coord_out, array_out in zip(out, expected, strict=True):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_core/test_coords.py` at line 2174, Update the zip call in the loop over coord_out and array_out to pass strict=True explicitly, preserving the existing iteration while documenting that out and expected must have equal lengths.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_core/test_coords.py`:
- Line 2174: Update the zip call in the loop over coord_out and array_out to
pass strict=True explicitly, preserving the existing iteration while documenting
that out and expected must have equal lengths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57d13c59-f0b5-4258-8d00-4e2b5bf86954
📒 Files selected for processing (5)
dascore/core/coords.pydascore/units.pydocs/tutorial/coords.qmdtests/test_core/test_coords.pytests/test_io/test_dasdae/test_dasdae.py
Coord data is read-only, and numpy enforces that for copyto, put, putmask, place, and the out parameter, so the custom rejection of those is removed in favor of numpy's own ValueError. Only ufunc.at needs an explicit guard since it ignores the flag.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7142452095
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| args = _map_nested(self._to_operand, args) | ||
| kwargs = _map_nested(self._to_operand, kwargs) |
There was a problem hiding this comment.
When a NumPy array function requires compatible units, such as np.clip(meter_coord, 3, 7) or np.concatenate([meter_coord, raw_values]), these calls invoke _to_operand without the coordinate's units. The raw bounds/values therefore remain dimensionless while the coordinate becomes a Pint quantity, causing Pint to reject the operation instead of treating the raw values as meters as promised for unit-matched operations. Array-function arguments need the same shared-unit handling used for matched ufuncs.
Useful? React with 👍 / 👎.
| if dtype_time_like(self.dtype) and func in _REDUCING_ARRAY_FUNCS: | ||
| out = _reduce_time_like(func, self.data) | ||
| return _wrap_array_op_output(out[0] if out.size == 1 else out, self.units) |
There was a problem hiding this comment.
For time-like coordinates, this branch discards every argument except the input data. Consequently calls such as np.mean(coord, where=mask) ignore the mask, and reductions on multidimensional coordinates with axis=... reduce the entire array; keepdims=True is also ignored. The time-aware reduction path must preserve applicable positional and keyword reduction arguments.
Useful? React with 👍 / 👎.
| """Implement numpy's array protocol (eg np.concatenate([coord1])).""" | ||
| if any(not issubclass(x, (BaseCoord, *_ARRAY_OP_TYPES)) for x in types): | ||
| return NotImplemented | ||
| if func in _MUTATING_ARRAY_FUNCS or kwargs.get("out") is not None: |
There was a problem hiding this comment.
This rejects mutating array functions whenever a coordinate appears anywhere in the call, even when a different array is the mutation target. For example, np.copyto(destination_array, coord) and np.putmask(destination_array, mask, coord) only read the coordinate but now raise ParameterError. The immutability check should reject these functions only when the coordinate is the destination being modified.
Useful? React with 👍 / 👎.
| if out.units.dimensionless: | ||
| out, units = out.to("dimensionless").magnitude, None |
There was a problem hiding this comment.
This treats every dimensionless Pint unit as a canceled unit and normalizes it away, including meaningful units already attached to the coordinate such as percent or radian. For example, adding 1 to a coordinate whose value is 50 with units percent produces the unitless value 0.51 rather than a coordinate containing 51 percent. Only dimensionless units produced by cancellation should be normalized and dropped; unit-preserving operations must retain the coordinate's original dimensionless unit and magnitude scale.
Useful? React with 👍 / 👎.
| operands = [_to_magnitude(x, units) for x in operands] | ||
| # When operands are quantities pint performs the unit algebra | ||
| # (eg m * m -> m ** 2) and raises on invalid ops (eg m + s). | ||
| out = getattr(ufunc, method)(*operands, **kwargs) |
There was a problem hiding this comment.
For ufunc methods, only positional operands are converted to shared magnitudes, while unit-bearing keyword operands are passed through unchanged. Thus a compatible call such as np.add.reduce(meter_coord, initial=100 * cm) sends a centimeter Quantity into a numeric reduction over meter magnitudes and raises instead of converting the initial value to 1 meter. Quantity-valued method keywords such as initial need conversion alongside the positional operands.
Useful? React with 👍 / 👎.
| np.min, | ||
| np.nanmin, | ||
| np.max, | ||
| np.nanmax, | ||
| } |
There was a problem hiding this comment.
np.average is absent from the time-like reduction set, so np.average(datetime_coord) bypasses _reduce_time_like and invokes NumPy directly on the raw datetime64 array. NumPy cannot average absolute datetimes through its ordinary add/divide reduction, so this common mean operation raises even though the equivalent np.mean(datetime_coord) is handled. Include np.average in the time-aware path, including its weights handling.
Useful? React with 👍 / 👎.
| # Types which coords know how to combine with in array operations. | ||
| _ARRAY_OP_TYPES = (np.ndarray, np.generic, numbers.Number, Quantity, list, tuple) |
There was a problem hiding this comment.
The supported operand types include Pint Quantity but omit Pint Unit, even though DASCore exposes get_unit and commonly forms quantities with expressions such as array * get_unit("m"). As a result, coord * get_unit("s") defers to Pint and either returns a bare Quantity or fails rather than returning a coordinate with combined units, contrary to the new array-like arithmetic contract. Pint Unit operands should be handled explicitly like other unit-bearing operands.
Useful? React with 👍 / 👎.
| # Time-like coords operate on raw values; pint knows nothing of | ||
| # numpy's datetime64/timedelta64. | ||
| if dtype_time_like(self.dtype): | ||
| return data |
There was a problem hiding this comment.
For a time-like coordinate, this early return discards every operand's Pint units and forwards only its magnitude to NumPy. On a datetime64[ns] coordinate, coord + 1 * second is therefore evaluated as datetime_array + 1 and advances by one nanosecond rather than one second; an incompatible quantity such as 1 * meter can likewise be interpreted as a raw integer instead of raising UnitError. Quantity operands must be converted to an appropriate timedelta64 or rejected before taking the raw time-like path.
Useful? React with 👍 / 👎.
Merging this PR will degrade performance by 11.55%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
|
✅ Documentation built: |
1 participant
Description
Fixes #566, coordinates now behave like arrays for arithmetic and numpy operations.
Coordinates implement the numpy ufunc and array-function protocols along with the arithmetic dunders. Rather than reimplementing unit logic, operands are converted to pint quantities and pint performs the unit algebra, so units reflect the operation performed instead of being carried over from the left operand.
Semantics, which follow what
Patchalready does where they overlap:coord + 1work as described in the issue. Elsewhere plain numbers are dimensionless, socoord / 2keeps its units.m + s) raise aUnitError, as does an operation which requires dimensionless input (np.expon a coord with units).np.mean,np.linalg.norm) return a scalar, as aQuantitywhen the coordinate has units. A zero-dimensional coordinate isn't useful.np.greater) return arrays, since the point of a mask is to index other arrays.time_coord + np.timedelta64(1, "s")works as before, and reductions (np.mean(time_coord)) go through the existing_reduce_time_likesince numpy can't average absolute times.ValueError: assignment destination is read-onlyfor operations which write into a coordinate (np.copyto,np.put,out=, ...).np.add.atis the exception: it ignores the flag (numpy 2.4.6) and would really mutate the coordinate, so it raises aParameterError, as it already does for patches.reduce/accumulate/outer) convert operands to the shared units first; those whose resulting units are ambiguous (np.multiply.reduce) raise aUnitErrorrather than silently dropping units, since pint does not implement these methods.coord > 4 * m) return masks.__eq__is deliberately left as pydantic model equality, since coordinate equality is used internally (align, spool equality) and is not element-wise.One test outside the new code changed:
test_roundtrip_datetime_coordcallednp.zeros_like(dist)on a coordinate and passed the result toto_datetime64. Now that numpy functions return coordinates, that call needsdist.values.Checklist
I have (if applicable):