-
Notifications
You must be signed in to change notification settings - Fork 132
Description
While reviewing the vector utility functions in movement/utils/vector.py, I noticed a couple of places where operations bypass xarray’s recommended patterns. These currently work, but they rely on implicit NumPy behaviour and may not be ideal for maintainability or compatibility with lazy backends (e.g. dask).
1. Use of np.arctan2 directly in compute_signed_angle_2d
In compute_signed_angle_2d() the signed angle is computed as:
angles = np.arctan2(cross, dot)
Since cross and dot are xarray.DataArray objects, calling the NumPy function directly relies on implicit behaviour and may drop metadata depending on broadcasting. Immediately afterwards the code asserts:
assert isinstance(angles, xr.DataArray)
A more idiomatic xarray approach would be:
angles = xr.apply_ufunc(np.arctan2, cross, dot)
This ensures coordinate preservation and improves compatibility with lazy evaluation backends.
2. Direct access to .values in cart2pol
In cart2pol() the following line appears:
phi = xr.where(np.isclose(rho.values, 0.0, atol=1e-9), 0.0, phi)
Using .values bypasses xarray's abstraction layer. It would be preferable to operate directly on the DataArray:
phi = xr.where(np.isclose(rho, 0.0, atol=1e-9), 0.0, phi)
This maintains xarray semantics and avoids unnecessary conversion to NumPy arrays.
Summary
Both cases currently function correctly but could be made more consistent with xarray best practices by:
- Using
xr.apply_ufuncfor NumPy operations onDataArrayobjects - Avoiding direct
.valuesaccess where possible
I would be happy to open a PR implementing these improvements if this approach is acceptable.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status