Skip to content

Propagate the units of the convolution kernel. - #35

Merged
roytsmart merged 1 commit into
mainfrom
convolve-quantity-kernel
Aug 20, 2026
Merged

Propagate the units of the convolution kernel.#35
roytsmart merged 1 commit into
mainfrom
convolve-quantity-kernel

Conversation

@roytsmart

Copy link
Copy Markdown
Collaborator

convolve() is annotated kernel: np.ndarray | u.Quantity, but only the
unit of array was ever unwrapped. A Quantity kernel reached the compiled
kernels still wrapped, where Numba treats it as a plain array, and the
outcome depended on whether array had a unit too:

import numpy as np, astropy.units as u, ndfilters

a = np.arange(7.0) * u.mm
k = (np.ones(3) / 3) * u.s

ndfilters.convolve(a, k).unit             # mm  -- the kernel unit is dropped
ndfilters.convolve(np.arange(7.0), k)     # AttributeError: 'numpy.ndarray'
                                          # object has no attribute 'unit'

The kernel is now unwrapped alongside the array and the two units are
multiplied together, so those give mm s and s respectively. A
convolution is a sum of products, so the product of the units is the unit of
the result.

Tests

Added a case over the four combinations of a unitless, dimensionless, and
dimensional array and kernel, asserting the exact unit of the result and
that the values match the unitless convolution. The existing suite covers a
Quantity array with a bare kernel.

Verified locally: pytest passes (2883 tests, up from 2875), pyright reports
0 errors, and black --check and ruff check ndfilters/ are clean.

🤖 Generated with Claude Code

`convolve()` is annotated to accept a `Quantity` kernel but only ever
unwrapped the unit of `array`. A `Quantity` kernel therefore reached the
compiled kernels still wrapped, where Numba treats it as a plain array:
with a `Quantity` array the unit of the kernel was silently dropped, and
with a bare array the call failed with an `AttributeError`.

Unwrap the kernel alongside the array and multiply the two units together.
The convolution is a sum of products, so the product of the units is the
unit of the result.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (9ee208d) to head (2fb4750).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #35   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines          651       677   +26     
=========================================
+ Hits           651       677   +26     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@roytsmart
roytsmart merged commit d71d84c into main Aug 20, 2026
18 checks passed
@roytsmart
roytsmart deleted the convolve-quantity-kernel branch August 20, 2026 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant