Fix: change convention for Downsample2d default sigma

[mrava87]

This PR changes the convention for the default sigma in Downsample2D choosing a value where the frequency spectrum of the gaussian filter is -10dB at the new Nyquist frequency. The old convention was somehow empirical and not 'fixed' for different downsampling factors.

[mrava87]

@claude review.

[claude]

Claude finished @mrava87's task in 2m 45s —— View job

---

Claude finished the review

Todo list:

• Gather context (PR diff, changed files)
• Review pylops/signalprocessing/downsample2d.py changes
• Review pytests/test_downsample.py changes
• Review docs/source/api/others.rst changes
• Post review feedback

---

Summary

The new standard_deviation_from_attenuation(factor, attenuation) helper derives sigma from a physically meaningful criterion (target dB attenuation of the Gaussian's frequency response at the new Nyquist frequency), replacing the empirical (factor - 1) / 2. I verified the derivation by hand:

For a Gaussian with std σ, the continuous-frequency response is H(f) = exp(-2π²σ²f²) (frequency normalized to sample rate = 1). Evaluated at the new Nyquist f = 0.5/factor and converted to dB (20·log10(H) = -A), solving for σ gives exactly docstring's -σ²π²/(2f²) = ln(10^(-A/20)), which matches downsample2d.py:54. The math and docstring are correct and consistent.

Findings

1. Behavior change for factors=1 (no decimation) — worth confirming intentional (pylops/signalprocessing/downsample2d.py:224-225)
When factors=1, the old convention gave sigma = 0 (pure identity, no smoothing — correct, since there's no aliasing risk with no decimation). The new default gives standard_deviation_from_attenuation(1, 10) ≈ 0.483, so Downsample2D now applies non-trivial Gaussian smoothing by default even when no downsampling occurs. This isn't exercised by the updated tests: test_Downsample2D_scipy only parametrizes over par1/par2/par1j/par2j (factors 3, (2,4)), skipping par3 (factors=1); test_Downsample2D_inverse does use factors=1 but passes an explicit sigma=0.6, bypassing the default entirely. If unintentional, consider special-casing sigma=0 when factor==1; if intentional, it may be worth a one-line note in the docstring since it's a behavior change from the previous convention.
Fix this →

2. New public function not added to __all__ (pylops/signalprocessing/downsample2d.py:1)
standard_deviation_from_attenuation is documented via autosummary in docs/source/api/others.rst, but the module's __all__ still only contains ["Downsample2D"], and pylops/signalprocessing/__init__.py's __all__ (which drives from .downsample2d import *) doesn't include it either — so it isn't reachable as pylops.signalprocessing.standard_deviation_from_attenuation. This is inconsistent with the existing convention elsewhere in the package (e.g. pylops/signalprocessing/patch2d.py, which lists its public helper functions like patch2d_design alongside the operator class in __all__).
Fix this →

3. Minor: magic number 10 duplicated (pylops/signalprocessing/downsample2d.py:225)
The default -10dB attenuation value is hardcoded in the __init__ call (standard_deviation_from_attenuation(f, 10)) and repeated in prose in the docstring/Notes. A small module-level constant (e.g. _DEFAULT_ATTENUATION_DB = 10.0) would keep the two in sync if the default is ever revisited. Not blocking.

Notes / limitations

• I could not execute make tests/pytest/python in this sandboxed environment (command execution requires interactive approval not available here), so I verified the math analytically by hand rather than by running the new test. The reasoning in test_Downsample2D_scipy (pytests/test_downsample.py:128) correctly mirrors the new default formula.
• Docs (docs/source/api/others.rst) and tests were updated consistently with the source change; no issues found there beyond the __all__ point above.

Overall this is a solid, well-documented improvement with correct math. The factors=1 default-smoothing behavior change (#1) is the main thing I'd want the author to confirm is intentional before merge.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 duplication

Metric	Results
Duplication	0

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[mrava87]

@claude review again.

[claude]

Claude finished @mrava87's task in 1m 7s —— View job

---

Claude's review (updated)

Todo list:

• Gather context (latest diff, changed files since last review)
• Check whether prior review findings were addressed
• Review current state of pylops/signalprocessing/downsample2d.py
• Review pytests/test_downsample.py
• Review docs/source/api/others.rst
• Post updated review feedback

---

Summary

Commit 0a92881 addresses the two substantive points from the previous review round. Verified against origin/dev...HEAD:

1. factors=1 default smoothing — fixed (pylops/signalprocessing/downsample2d.py:227-230)

sigma = tuple(
    standard_deviation_from_attenuation(f, 10) if f > 1 else 0
    for f in factors
)

Now correctly falls back to sigma=0 when a given axis has factor==1, restoring identity behavior for that axis (no unwanted smoothing when there's no aliasing risk). _gaussian_kernel1d already special-cases sigma==0.0 → [1.], so this composes cleanly.

2. __all__ reachability — fixed (pylops/signalprocessing/downsample2d.py:1-4)
standard_deviation_from_attenuation is now in the module's __all__, so from .downsample2d import * (in pylops/signalprocessing/__init__.py:63) brings it into the pylops.signalprocessing namespace. Note it's still not in the package-level __all__ list in pylops/signalprocessing/__init__.py:85-123 — but I confirmed that's consistent with existing precedent (e.g. patch2d_design is similarly omitted from that list despite being in patch2d.py's own __all__), so this is fine as-is and matches the codebase convention.

3. Magic number 10 duplicated — still present (pylops/signalprocessing/downsample2d.py:229), as previously noted as non-blocking.

Minor remaining gap (not blocking)

test_Downsample2D_scipy (pytests/test_downsample.py:119) is still only parametrized over par1/par2/par1j/par2j (factors 3, (2,4)), so the new factor==1 → sigma=0 branch isn't directly exercised by the scipy-comparison test (it is however covered indirectly by test_Downsample2D, which does dot-test par3 with factors=1). Optional: add par3 to test_Downsample2D_scipy's parametrize list to explicitly assert the sigma=0 fallback matches scipy's ungaussianed identity output.

Notes / limitations

• Command execution (pytest) wasn't available in this sandboxed run, so this was verified by reading code/tests rather than executing them.

Overall, the PR looks solid — both actionable findings from the previous review are addressed correctly, and the math/docs remain consistent.