Skip to content

FIX: Fix incorrect behavior for snip when smoothing or using filter_order > 2 - #94

Merged
derb12 merged 5 commits into
developmentfrom
update_snip
Sep 12, 2026
Merged

derb12 merged 5 commits into
developmentfrom
update_snip

Conversation

@derb12

@derb12 derb12 commented Sep 12, 2026

Copy link
Copy Markdown
Owner

Description

Closes issue #93. Fixes 2 bugs in snip that were found when adding validation tests against ROOT's implementation, which was written by Morháč':

  • When smoothing, the smoothed values should have been used for the filter calculations, rather than just in the comparison test.
  • For filter orders > 2, the edge indices of the filters were incorrectly set as i +- (order//2) * (i / (order//2)), ie. 0 and 2i, rather than i +- (order//2) * (i // (order//2)).

Added a validation test against data files output by ROOT's implementation. Also added a docstring example for snip since I was here, to finally start following up on #48.

Type of Pull Request

  • Bug Fix
  • New Feature
  • Miscellaneous Changes (refactor, code improvements, etc.)
  • Documentation or Example Programs

Pull Request Checklist

  • New code and/or documentation is valid for use with the BSD 3-clause license.
  • New code is fully documented with docstrings that follow Numpy style,
    if applicable.
  • New code follows PEP 8 standards as closely as possible, if applicable.
  • Added/updated tests and ensured they pass locally, if applicable.
  • Verified that documentation builds locally, if applicable.

Subtle difference between pybaselines and ROOT for filter order 4, which isn't really easy to figure out, so ignoring higher orders for now.
Filters should use the smoothed data rather than only using the smoothed data in the comparison replacement.
Outer indices for filters > 2 were incorrectly at 0 and -1 rather than i += (order//2) * (i // (order//2)). Also modified end indexing to always use `num_y - i` rather than `-i`, just for readability.
@derb12

derb12 commented Sep 12, 2026

Copy link
Copy Markdown
Owner Author

Unrelated to this PR, but the min dependancy test is giving the following warning for tests/two_d/test_whittaker.py::TestDrPLS::test_masking: "SparseEfficiencyWarning: Changing the sparsity structure of a csr_matrix is expensive". I'm guessing that started in #83.

I guess when sparse matrices are used, they don't like zeros within the weights from masking during the W @ P calc; I remember I had to add a shim for the 2D aspls version years ago to add eps to avoid the same warning. Not sure it's worth addressing since sparse matrices are only used if the scipy version is < 1.12, and the min versions will get bumped before the next release. According to SPEC 0, technically scipy 1.15 is the oldest I should be supporting, so maybe I could start following that?

@derb12
derb12 merged commit 83f0e57 into development Sep 12, 2026
11 checks passed
@derb12
derb12 deleted the update_snip branch September 12, 2026 17:31
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