Open
Conversation
Reviewer's GuideIntroduces a pluggable interpolation interface with a new NaN-aware interpolation mode for angular averaging and power spectrum calculations, refactors how interpolation weights and sumweights are computed, and extends tests to cover NaN-aware behaviour and consistency between interpolated and non-interpolated results. Sequence diagram for angular_average and NaN-aware interpolation flowsequenceDiagram
participant get_power
participant angular_average_nd
participant angular_average
participant _field_average_interpolate
participant linear_interp
participant nan_aware_interp
participant RegularGridInterpolator
get_power->>angular_average_nd: call (field, coords, interpolation_method, ...)
angular_average_nd->>angular_average: call per-slice
alt interpolation_method is str
angular_average->>angular_average: resolve string
alt 'linear'
angular_average->>angular_average: set interpolation_method = linear_interp
else 'nan-aware'
angular_average->>angular_average: warn about slower runtime
angular_average->>angular_average: set interpolation_method = nan_aware_interp
else unknown
angular_average-->>get_power: raise ValueError
end
else interpolation_method is callable or None
angular_average->>angular_average: validate callable/None
end
alt interpolation_method is None
angular_average->>angular_average: bin without interpolation
angular_average-->>angular_average_nd: avg, bins, var, sumweights
else interpolation_method is callable
angular_average->>_field_average_interpolate: coords, field, bins, weights, sample_coords, r_n, interp_fn=interpolation_method
_field_average_interpolate->>_field_average_interpolate: rescale field (mean/std)
alt interp_fn == linear_interp
_field_average_interpolate->>linear_interp: coords, rescaled_field, sample_points
linear_interp->>RegularGridInterpolator: construct (field)
linear_interp->>RegularGridInterpolator: sample
RegularGridInterpolator-->>linear_interp: interpolated_field
linear_interp-->>_field_average_interpolate: interped_field
else interp_fn == nan_aware_interp
_field_average_interpolate->>nan_aware_interp: coords, rescaled_field, sample_points
nan_aware_interp->>nan_aware_interp: valid = isfinite(field)
nan_aware_interp->>RegularGridInterpolator: construct (field_filled)
nan_aware_interp->>RegularGridInterpolator: sample -> numerator
nan_aware_interp->>RegularGridInterpolator: construct (valid_mask)
nan_aware_interp->>RegularGridInterpolator: sample -> denominator
nan_aware_interp->>nan_aware_interp: result = numerator/denominator (NaN if denom==0)
nan_aware_interp-->>_field_average_interpolate: interped_field
else custom interp_fn
_field_average_interpolate->>_field_average_interpolate: call user interp_fn
end
_field_average_interpolate->>_field_average_interpolate: average over bins to avged_field
_field_average_interpolate-->>angular_average: avged_field
angular_average->>angular_average: compute sumweights from original grid (digitize coord_mags)
angular_average-->>angular_average_nd: avg, bins, var=None, sumweights
end
angular_average_nd->>angular_average_nd: assemble outputs across slices
angular_average_nd-->>get_power: P, k, var, sumweights, extra_freq (tuple)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The docstrings for
angular_average,angular_average_nd, andget_powerrefer to:func:_nan_aware_interp`` but the actual function is namednan_aware_interp; update these references to avoid confusing users and broken Sphinx links. - In
nan_aware_interp,np.where(denominator > 0, numerator / denominator, np.nan)will still computenumerator / denominatorand can emitRuntimeWarning: invalid value encountered in divide; consider preallocatingresult = np.full_like(numerator, np.nan)and only performing the division onmask = denominator > 0to avoid unnecessary work and warnings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The docstrings for `angular_average`, `angular_average_nd`, and `get_power` refer to `:func:`_nan_aware_interp`` but the actual function is named `nan_aware_interp`; update these references to avoid confusing users and broken Sphinx links.
- In `nan_aware_interp`, `np.where(denominator > 0, numerator / denominator, np.nan)` will still compute `numerator / denominator` and can emit `RuntimeWarning: invalid value encountered in divide`; consider preallocating `result = np.full_like(numerator, np.nan)` and only performing the division on `mask = denominator > 0` to avoid unnecessary work and warnings.
## Individual Comments
### Comment 1
<location path="src/powerbox/tools.py" line_range="121-130" />
<code_context>
+
+ * ``'linear'`` — resolved to :func:`linear_interp`, which wraps
+ :class:`~scipy.interpolate.RegularGridInterpolator`.
+ * ``'nan-aware'`` — resolved to :func:`_nan_aware_interp`, which
+ uses normalised convolution (two ``RegularGridInterpolator``
+ calls) so that NaN grid cells do not poison neighbouring
+ sample points. This is mainly useful when the requested k
+ range extends to very low values (edge bins close to masked
+ k_i = 0 planes), where the standard trilinear stencil would
+ produce NaN because some of its corners lie on those planes.
+ If the k range starts at higher values, ``'linear'`` is
+ sufficient to fill in bins between valid grid points and will
+ be faster.
+ * A **callable** with signature
+ ``(coords, field, sample_points) -> result``, where *coords*
+ is a tuple of 1-D coordinate arrays, *field* is the N-D data
</code_context>
<issue_to_address>
**issue (typo):** The docstring references `_nan_aware_interp` but the actual helper is `nan_aware_interp`.
The narrative here should reference `nan_aware_interp` (without the leading underscore) to match the actual helper name. Please update this (and the other updated docstrings) so the documented name aligns with the public function.
Suggested implementation:
```python
* ``'linear'`` — resolved to :func:`linear_interp`, which wraps
:class:`~scipy.interpolate.RegularGridInterpolator`.
* ``'nan-aware'`` — resolved to :func:`nan_aware_interp`, which
uses normalised convolution (two ``RegularGridInterpolator``
```
Search the rest of `src/powerbox/tools.py` (and any related modules updated in this PR) for docstrings or documentation text that reference ``_nan_aware_interp`` and update them to reference ``nan_aware_interp`` instead, so that all public-facing documentation matches the actual helper name.
</issue_to_address>
### Comment 2
<location path="tests/test_tools.py" line_range="168" />
<code_context>
+ message="invalid value encountered in divide",
+ category=RuntimeWarning,
+ )
+ p_k_lin, *_ = angular_average(
+ field=P,
+ coords=freq,
+ bins=10,
+ interpolation_method="nan-aware",
+ weights=weights,
+ interp_points_generator=regular_angular_generator(angular_resolution=0.4),
+ log_bins=True,
+ bins_upto_boxlen=True,
+ )
+
+ assert np.all(p_k_lin == 1.0)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Use tolerant equality for floating-point result instead of exact `== 1.0` in the nan-aware interpolation test.
Since `p_k_lin` comes from interpolation and spherical averaging, exact equality to `1.0` is likely to be numerically fragile across platforms or small implementation changes. Using a tolerant check like `np.allclose(p_k_lin, 1.0, rtol=1e-6, atol=1e-8)` will keep the test robust while still validating the expected behaviour.
```suggestion
assert np.allclose(p_k_lin, 1.0, rtol=1e-6, atol=1e-8)
```
</issue_to_address>
### Comment 3
<location path="tests/test_tools.py" line_range="649-660" />
<code_context>
+ P = r2**-1.0
+ bins = np.linspace(0, x.max(), 30)
+
+ avg_none, k_none, _, sw_none = angular_average(
+ P, [x, x], bins=bins, interpolation_method=None
+ )
+ with warnings.catch_warnings():
+ warnings.filterwarnings(
+ "ignore",
+ message="'nan-aware' interpolation uses two",
+ category=UserWarning,
+ )
+ avg_interp, k_interp, _, sw_interp = angular_average(
+ P, [x, x], bins=bins, interpolation_method=interpolation_method
+ )
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert that interpolated runs produce identical `k` bins and consistent sumweights to the non-interpolated baseline.
Given that `sumweights` is now derived from the grid and bins may be reshaped/collapsed, extend the test to check that `k_interp` and `k_none` are equal (e.g. `np.allclose`) and that `sw_interp` matches `sw_none` (exact or within a small tolerance). This will directly exercise the new `sumweights` logic and catch any interpolation-induced changes to bin definitions.
```suggestion
avg_none, k_none, _, sw_none = angular_average(
P, [x, x], bins=bins, interpolation_method=None
)
with warnings.catch_warnings():
warnings.filterwarnings(
"ignore",
message="'nan-aware' interpolation uses two",
category=UserWarning,
)
avg_interp, k_interp, _, sw_interp = angular_average(
P, [x, x], bins=bins, interpolation_method=interpolation_method
)
# Interpolated runs should not alter the k-bin definitions or sumweights
assert np.allclose(k_interp, k_none)
assert np.allclose(sw_interp, sw_none)
```
</issue_to_address>
### Comment 4
<location path="tests/test_tools.py" line_range="637-638" />
<code_context>
angular_average_nd(field=P, coords=r2, weights=P[1:], bins=4)
+
+
+@pytest.mark.parametrize("interpolation_method", ["linear", "nan-aware"])
+class TestInterpSimilarToNoInterp:
+ """Check that interpolated angular averages are close to non-interpolated ones."""
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add targeted tests for `nan-aware` vs `linear` behaviour in the presence of NaNs.
The new parametrised tests are good for regression coverage but don’t directly check the key `nan-aware` behaviour of ignoring NaN corners instead of propagating them like `RegularGridInterpolator`.
Please add a focused unit test that:
- Uses a small 2D/3D grid with only some stencil corners as NaN.
- Interpolates at a point whose stencil includes both NaN and finite corners.
- Asserts that `linear` returns NaN while `nan-aware` returns the finite, correctly normalised value from the valid corners.
- Also covers the case where all corners are NaN, asserting that `nan-aware` returns NaN.
This will verify the documented NaN-handling semantics and guard against regressions in this logic.
Suggested implementation:
```python
class TestNanAwareVsLinearInterpolation:
"""Focused tests for NaN handling differences between linear and nan-aware interpolation."""
def test_mixed_nan_and_finite_corners(self):
# 2D grid with one NaN corner
x = np.array([0.0, 1.0])
y = np.array([0.0, 1.0])
# Values at grid corners:
# (0,0)=1, (1,0)=3
# (0,1)=NaN, (1,1)=4
values = np.array(
[
[1.0, np.nan],
[3.0, 4.0],
]
)
# Interpolate at the center (0.5, 0.5) so all four corners participate in the stencil.
point = np.array([[0.5, 0.5]])
# Linear interpolation via RegularGridInterpolator should propagate NaNs
from scipy.interpolate import RegularGridInterpolator
lin_interp = RegularGridInterpolator((x, y), values, bounds_error=False)
lin_val = lin_interp(point)
assert np.isnan(lin_val)
# Nan-aware interpolation should ignore the NaN corner and renormalise the weights.
# At the center, all four corners have equal weight; nan-aware should average over
# only the finite corners: (1 + 3 + 4) / 3.
nan_aware_val = nan_aware_regular_grid_interpolator((x, y), values)(point)
assert np.isfinite(nan_aware_val)
assert np.allclose(nan_aware_val, (1.0 + 3.0 + 4.0) / 3.0)
def test_all_nan_corners(self):
# 2D grid where all corners are NaN
x = np.array([0.0, 1.0])
y = np.array([0.0, 1.0])
values = np.array(
[
[np.nan, np.nan],
[np.nan, np.nan],
]
)
point = np.array([[0.5, 0.5]])
from scipy.interpolate import RegularGridInterpolator
lin_interp = RegularGridInterpolator((x, y), values, bounds_error=False)
lin_val = lin_interp(point)
assert np.isnan(lin_val)
nan_aware_val = nan_aware_regular_grid_interpolator((x, y), values)(point)
# With all-NaN stencil, nan-aware should also return NaN
assert np.isnan(nan_aware_val)
def test_bins_upto_boxlen_warning():
assert np.isclose(bins.max(), xmax * np.sqrt(ndim))
```
1. Ensure that `nan_aware_regular_grid_interpolator` is imported into `tests/test_tools.py` (e.g. from the module where it is defined). The tests assume it has the signature `nan_aware_regular_grid_interpolator(grid, values)` and returns a callable like `RegularGridInterpolator`.
2. If the actual helper or factory for the nan-aware interpolator has a different name or signature, update the calls `nan_aware_regular_grid_interpolator((x, y), values)` accordingly (e.g. `tools.nan_aware_regular_grid_interpolator(...)` or similar).
3. If `scipy.interpolate.RegularGridInterpolator` is already imported at the top of the file, you may remove the local imports inside the tests and reuse the existing import instead.
4. If the implementation also supports 3D grids and you want explicit 3D coverage, you can add an analogous test method using a small 2×2×2 grid with mixed NaN/finite corners and the same assertions.
</issue_to_address>
### Comment 5
<location path="tests/test_tools.py" line_range="85-86" />
<code_context>
-@pytest.mark.parametrize("interpolation_method", [None, "linear"])
+@pytest.mark.parametrize("interpolation_method", [None, "linear","nan-aware"])
def test_angular_avg_nd_3(interpolation_method):
x = np.linspace(-3, 3, 400)
X, Y = np.meshgrid(x, x)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for invalid and custom `interpolation_method` values.
The implementation now accepts `interpolation_method` as `None`, `'linear'`, `'nan-aware'`, or a callable, and raises `ValueError` for unknown strings or unsupported types. Current tests only cover the valid string/`None` cases. To better cover the contract, please also add tests that:
- Use an unknown string (e.g. `'cubic'`) and assert the expected `ValueError`.
- Use a non-callable, non-string value (e.g. `123`) and assert the expected `ValueError`.
- Use a simple custom callable and verify it is invoked by `angular_average` / `angular_average_nd`.
This will exercise the error and custom-callable paths and better lock in the new behavior.
Suggested implementation:
```python
@pytest.mark.parametrize("interpolation_method", [None, "linear","nan-aware"])
def test_angular_avg_nd_3(interpolation_method):
x = np.linspace(-3, 3, 400)
X, Y = np.meshgrid(x, x)
P = r2**-1.0
P = np.repeat(P, 100).reshape(400, 400, 100)
freq = [x, x, np.linspace(-2, 2, 100)]
@pytest.mark.parametrize("invalid_interpolation_method", ["cubic"])
def test_angular_avg_nd_invalid_string_interpolation_method(invalid_interpolation_method):
x = np.linspace(-3, 3, 400)
X, Y = np.meshgrid(x, x)
P = r2**-1.0
P = np.repeat(P, 100).reshape(400, 400, 100)
freq = [x, x, np.linspace(-2, 2, 100)]
with pytest.raises(ValueError):
angular_average_nd(
freq,
P,
interpolation_method=invalid_interpolation_method,
)
@pytest.mark.parametrize("invalid_interpolation_method", [123])
def test_angular_avg_nd_invalid_type_interpolation_method(invalid_interpolation_method):
x = np.linspace(-3, 3, 400)
X, Y = np.meshgrid(x, x)
P = r2**-1.0
P = np.repeat(P, 100).reshape(400, 400, 100)
freq = [x, x, np.linspace(-2, 2, 100)]
with pytest.raises(ValueError):
angular_average_nd(
freq,
P,
interpolation_method=invalid_interpolation_method,
)
def test_angular_avg_nd_custom_callable_interpolation_method():
class _InterpolationCalled(RuntimeError):
"""Sentinel exception to verify custom interpolation is invoked."""
def custom_interpolation_method(*args, **kwargs):
# Raising a sentinel exception lets us assert that this callable
# is actually invoked by angular_average_nd without depending on
# its exact call signature or return contract.
raise _InterpolationCalled("custom interpolation method was called")
x = np.linspace(-3, 3, 400)
X, Y = np.meshgrid(x, x)
P = r2**-1.0
P = np.repeat(P, 100).reshape(400, 400, 100)
freq = [x, x, np.linspace(-2, 2, 100)]
with pytest.raises(_InterpolationCalled):
angular_average_nd(
freq,
P,
interpolation_method=custom_interpolation_method,
)
```
These tests assume:
1. `angular_average_nd` is already imported or available in this module (consistent with the existing `test_angular_avg_nd_3` usage).
2. The `(freq, P, interpolation_method=...)` call signature is valid. If the real signature requires additional arguments (e.g. `bins` or `weights`), mirror the exact call pattern used in `test_angular_avg_nd_3` inside the new tests so they exercise the same code path with different `interpolation_method` values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -146,6 +167,32 @@ def test_interp_w_weights(n): | |||
|
|
|||
| assert np.all(p_k_lin == 1.0) | |||
There was a problem hiding this comment.
suggestion (testing): Use tolerant equality for floating-point result instead of exact == 1.0 in the nan-aware interpolation test.
Since p_k_lin comes from interpolation and spherical averaging, exact equality to 1.0 is likely to be numerically fragile across platforms or small implementation changes. Using a tolerant check like np.allclose(p_k_lin, 1.0, rtol=1e-6, atol=1e-8) will keep the test robust while still validating the expected behaviour.
Suggested change
| assert np.all(p_k_lin == 1.0) | |
| assert np.allclose(p_k_lin, 1.0, rtol=1e-6, atol=1e-8) |
Comment on lines
+649
to
+660
| avg_none, k_none, _, sw_none = angular_average( | ||
| P, [x, x], bins=bins, interpolation_method=None | ||
| ) | ||
| with warnings.catch_warnings(): | ||
| warnings.filterwarnings( | ||
| "ignore", | ||
| message="'nan-aware' interpolation uses two", | ||
| category=UserWarning, | ||
| ) | ||
| avg_interp, k_interp, _, sw_interp = angular_average( | ||
| P, [x, x], bins=bins, interpolation_method=interpolation_method | ||
| ) |
There was a problem hiding this comment.
suggestion (testing): Also assert that interpolated runs produce identical k bins and consistent sumweights to the non-interpolated baseline.
Given that sumweights is now derived from the grid and bins may be reshaped/collapsed, extend the test to check that k_interp and k_none are equal (e.g. np.allclose) and that sw_interp matches sw_none (exact or within a small tolerance). This will directly exercise the new sumweights logic and catch any interpolation-induced changes to bin definitions.
Suggested change
| avg_none, k_none, _, sw_none = angular_average( | |
| P, [x, x], bins=bins, interpolation_method=None | |
| ) | |
| with warnings.catch_warnings(): | |
| warnings.filterwarnings( | |
| "ignore", | |
| message="'nan-aware' interpolation uses two", | |
| category=UserWarning, | |
| ) | |
| avg_interp, k_interp, _, sw_interp = angular_average( | |
| P, [x, x], bins=bins, interpolation_method=interpolation_method | |
| ) | |
| avg_none, k_none, _, sw_none = angular_average( | |
| P, [x, x], bins=bins, interpolation_method=None | |
| ) | |
| with warnings.catch_warnings(): | |
| warnings.filterwarnings( | |
| "ignore", | |
| message="'nan-aware' interpolation uses two", | |
| category=UserWarning, | |
| ) | |
| avg_interp, k_interp, _, sw_interp = angular_average( | |
| P, [x, x], bins=bins, interpolation_method=interpolation_method | |
| ) | |
| # Interpolated runs should not alter the k-bin definitions or sumweights | |
| assert np.allclose(k_interp, k_none) | |
| assert np.allclose(sw_interp, sw_none) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #99 +/- ##
==========================================
+ Coverage 93.84% 94.12% +0.27%
==========================================
Files 5 5
Lines 569 596 +27
==========================================
+ Hits 534 561 +27
Misses 35 35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a new NaN-tolerant interpolation mode for angular averaging / power spectrum computation, and refactors interpolation handling to accept named modes or custom callables while adjusting how sumweights are computed during interpolation.
Changes:
- Introduces
nan-awareinterpolation (normalized-convolution style) alongside the existing linear interpolation path. - Refactors interpolation plumbing to use interpolation callables (
linear_interp,nan_aware_interp) and updates documentation accordingly. - Expands tests to cover
nan-awareand to compare interpolated vs non-interpolated results.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/powerbox/tools.py |
Adds nan_aware_interp, refactors interpolation to accept callables/strings, adjusts sumweights computation, and tweaks get_power return shaping. |
tests/test_tools.py |
Extends interpolation coverage (including nan-aware) and adds similarity tests against the non-interpolated baseline. |
Comments suppressed due to low confidence (15)
tests/test_tools.py:729
k_interpis unused here (RuffF841). Replace it with_unless you intend to compare the bin centres between interpolated and non-interpolated runs.
avg_interp, k_interp, *_ = angular_average_nd(
field=P,
coords=freq[:2],
bins=30,
interpolation_method=interpolation_method,
src/powerbox/tools.py:750
- The
angular_average_nddocstring also refers to:func:_nan_aware_interp`` but the function exported here isnan_aware_interp. Updating this keeps the documentation consistent with the actual API.
* ``'linear'`` — standard trilinear interpolation via
:func:`linear_interp`.
* ``'nan-aware'`` — NaN-tolerant interpolation via
:func:`_nan_aware_interp` (slower, uses two interpolator
calls). Most useful when the k range reaches very low values
near masked k_i = 0 planes; for higher k ranges ``'linear'``
is sufficient and faster.
* A **callable** ``(coords, field, sample_points) -> result``
src/powerbox/tools.py:1189
get_powerdocstring references:func:_nan_aware_interp`` but the implementation isnan_aware_interp. Fixing the name avoids broken cross-references in generated docs.
* ``'linear'`` — standard trilinear interpolation via
:func:`linear_interp`.
* ``'nan-aware'`` — NaN-tolerant interpolation via
:func:`_nan_aware_interp` (slower, uses two interpolator
calls). Most useful when
the requested k range extends to very low values (edge bins
near masked k_i = 0 planes) where standard trilinear
interpolation would return NaN. If the k range starts at
src/powerbox/tools.py:1346
return_sumweightsis documented as controlling whether sumweights are returned, but it is not referenced anywhere inget_powerand the function always returns a 4- or 5-tuple includingsumweights. This is confusing for API consumers; either implement the flag (e.g., returnNone/omit the element when False) or deprecate/remove the parameter and update the docstring accordingly.
# Build return: (P, k, var, sumweights, [extra_freq])
ret = [res[0], res[1], res[2], res[3]]
if res_ndim < dim:
ret.append(freq[res_ndim:])
return tuple(ret) if len(ret) > 1 else ret[0]
tests/test_tools.py:684
- Similarly here,
k_none/sw_noneare assigned but unused, which Ruff will flag asF841. Replace unused tuple elements with_to keep the test lint-clean.
avg_none, k_none, _, sw_none = angular_average(
P, [x, x, x], bins=bins, interpolation_method=None
)
src/powerbox/tools.py:123
- Docstring references
:func:_nan_aware_interp`` but the implementation is namednan_aware_interp(no leading underscore). This will create broken documentation links and confusion for users; update the references to the correct callable name.
* ``'linear'`` — resolved to :func:`linear_interp`, which wraps
:class:`~scipy.interpolate.RegularGridInterpolator`.
* ``'nan-aware'`` — resolved to :func:`_nan_aware_interp`, which
uses normalised convolution (two ``RegularGridInterpolator``
calls) so that NaN grid cells do not poison neighbouring
src/powerbox/tools.py:1339
get_powercollapsesbinsandsumweightsto 1D by taking the first slice along the un-averaged dimensions. This is only correct if those arrays are guaranteed identical across the remaining dimensions; that may not hold if callers passk_weights/mask arrays that vary across the un-averaged dimensions (whichangular_average_ndsupports). Consider only collapsing when weights are known to be independent of the remaining dims (e.g.,k_weights.ndim == res_ndim) or assert/equivalence-check before collapsing.
# When averaging over fewer than all dimensions, the bins and sumweights
# arrays from angular_average_nd have shape (n_bins, *remaining_dims).
# Since the bins (and typically the sumweights) are identical across the
# remaining dimensions, collapse them to 1D.
if res_ndim < dim:
for idx in (1, 3): # bins and sumweights
arr = res[idx]
if arr is not None and arr.ndim > 1:
res[idx] = arr[(slice(None),) + (0,) * (arr.ndim - 1)]
src/powerbox/tools.py:708
- Same typing issue as in
angular_average:interpolation_methodis annotated ascallable | str | None, butcallableis the built-in predicate, not a type. For correct typing (and to support tools like mypy/pyright), switch this tocollections.abc.Callable/typing.Callable.
weights: np.ndarray | float = 1.0,
interpolation_method: callable | str | None = None,
**kwargs,
tests/test_tools.py:693
k_interp/sw_interpare also unused here; Ruff will flag them asF841. Use_placeholders for the unused return values.
avg_interp, k_interp, _, sw_interp = angular_average(
P, [x, x, x], bins=bins, interpolation_method=interpolation_method
)
src/powerbox/tools.py:74
- The type annotations use the built-in
callable(e.g.,interpolation_method: callable | str | Noneandinterp_points_generator: callable[...]).callableis a function, not a typing type; this breaks static type checking and Sphinx/autodoc rendering. Consider importing and usingcollections.abc.Callable(ortyping.Callable) for these annotations (and similarly inangular_average_nd).
interpolation_method: callable | str | None = None,
interp_points_generator: callable[
[float], callable[[np.ndarray, int], tuple[np.ndarray, np.ndarray]]
] = None,
tests/test_tools.py:182
- This test suppresses
RuntimeWarning: invalid value encountered in divide, which appears to originate fromnan_aware_interpdoingnumerator/denominatorwhendenominator==0. Once the interpolation implementation avoids that warning (e.g., usingnp.divide(..., where=...)), it would be better to drop this filter so the test suite doesn’t mask unexpected numerical issues.
warnings.filterwarnings(
"ignore",
message="invalid value encountered in divide",
category=RuntimeWarning,
)
tests/test_tools.py:653
- These tests assign
k_none,sw_none,k_interp, andsw_interpbut never use them. With Ruff enabled in pre-commit, this will raiseF841(local variable assigned but never used). Consider replacing the unused variables with_placeholders (or asserting on them if they’re intended to be part of the check).
avg_none, k_none, _, sw_none = angular_average(
P, [x, x], bins=bins, interpolation_method=None
)
with warnings.catch_warnings():
warnings.filterwarnings(
tests/test_tools.py:660
k_interpandsw_interpare assigned but never used, which will be reported by Ruff asF841. Replace the unused elements with_placeholders (or assert on them if needed).
avg_interp, k_interp, _, sw_interp = angular_average(
P, [x, x], bins=bins, interpolation_method=interpolation_method
)
tests/test_tools.py:718
k_noneis assigned but never used (RuffF841). If the bin centres aren’t needed for this assertion, replacek_nonewith_.
avg_none, k_none, *_ = angular_average_nd(
field=P, coords=freq[:2], bins=30, interpolation_method=None
)
src/powerbox/tools.py:588
nan_aware_interpcomputesnumerator / denominatorinsidenp.where(...), which evaluates the division for all elements and can emitRuntimeWarning: invalid value encountered in dividewhendenominator == 0. To avoid noisy user-facing warnings (and the need to suppress them in tests), compute the division withnp.divide(..., where=denominator>0)or wrap the division innp.errstate(divide='ignore', invalid='ignore').
numerator = interp_num(sample_points)
denominator = interp_den(sample_points)
result = np.where(denominator > 0, numerator / denominator, np.nan)
return result
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
nan-aware(tentative name haha) that avoids NaNs when some of the bins in the trilinear interpolation scheme are NaN. This is useful when calculating the PS over the lowest k-modes available in a field while usingignore_zero_ki(because having all three k_i = 0 planes set to NaN adds a lot of NaNs to the field; then the traditional RegularGridInterpolator just sets the interpolated value to NaN as soon as any one bin among the neighbours is NaN; this nan-aware interpolation calls RegularGridInterpolator twice and fixes this issue. A warning is raised to notify the user that the nan-aware method is even slower (twice the runtime) than usual interpolation due to the two calls to RegularGridInterpolator. The docstrings have also been updated to include this information i.e. if you want a PS reasonably far away from the lowest k-mode, regular interpolation is the way to go!)Summary by Sourcery
Add a NaN-aware interpolation option and generalise interpolation handling for angular averaging and power spectrum utilities.
New Features:
Enhancements:
Tests: