Fix falsy pad-constant check in pad_sliding_window and correct window stat docstrings - #429
Merged
Conversation
…docs pad_sliding_window used a truthiness check on pad_const, so a pad constant of 0 (or 0.0) silently fell through to edge padding instead of constant padding. Every current caller pads with nan, so this is a latent bug with no change to computed features. Also corrects two misleading docstrings and replaces an obscure np.where loop with a direct boolean index. Adds unit tests for the previously untested window_stats module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ALN3xSiV2DPo5JFDRXa9sK
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens and clarifies the sliding-window statistics helpers used across JABS feature extraction by fixing a latent padding bug, correcting misleading docstrings, and adding a dedicated unit test module for window_stats.py.
Changes:
- Fixed
pad_sliding_windowto treatpad_const=0/0.0as a real constant (usingis not Noneinstead of a truthiness check). - Simplified
get_window_masksimplementation while correcting/clarifying mask polarity documentation (True= valid). - Added a new
pytestmodule covering padding, masking, and the windowed stats helpers (mean/median/std/min/max/skew/kurtosis).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/jabs/feature_extraction/window_operations/window_stats.py |
Fixes the falsy-constant padding bug, simplifies get_window_masks, and corrects docstrings. |
tests/feature_extraction/window_operations/test_window_stats.py |
Adds unit tests to prevent regressions and validate padding/masking/stat behavior. |
💡 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.
Reasoning
src/jabs/feature_extraction/window_operations/window_stats.pybacks every non-circular window feature in JABS (mean,median,std_dev,skew,kurtosis,min,max— see the_window_operationstable infeature_base_class.py:70-78), and it had no test module at all. Three defects were sitting in it:Latent bug — falsy pad constant.
pad_sliding_windowtestedif pad_const:to decide between constant padding and edge padding. Its own docstring says "None will pad with value at the edge", but a pad constant of0or0.0is falsy, so it silently took the edge-padding branch instead. This is the classic "truthiness where you meantis not None" bug. Every caller today passesnp.nan(truthy), so no computed feature value changes — hence noFEATURE_VERSIONbump. It's a trap for the next caller, not a live regression.Inverted docstring.
get_window_masksdocumented its return as "valid (0) and invalid (1)". The code returns the opposite polarity:Truemarks valid values. Both call sites confirm this —window_medianpassesmask=~window_maskstonp.ma.array(whereTruemeans masked-out), andwindow_min/window_maxpasswhere=window_masksto the reduction (whereTruemeans include). A reader trusting the docstring would invert the mask.Copy-paste docstring.
window_minwas documented as "Calculates a masked maximum of a window" / "sliding window maximum values".I looked at several other candidates —
TrackLabels.downsamplebinning rules, the LOGO split helpers inclassifier_utils.py, and the psd helpers insignal_stats.py— but those are either already well documented or would require a behavior change to "fix". This file had the highest ratio of real defect to blast radius, and it was the only one where a latent bug, two wrong docstrings, and zero test coverage all overlapped.Why this is safe: the only executable change is
if pad_const:→if pad_const is not None:, which can only differ whenpad_constis0/0.0, and no caller in the repository passes that. Theget_window_masksrewrite is provably equivalent:np.whereon a 1-D boolean returns a 1-tuple, so the oldforloop iterated exactly once over the index array and assigned the same rows the new boolean index assigns. Full suite: 825 passed, 200 skipped.Change
Logic changes
src/jabs/feature_extraction/window_operations/window_stats.pypad_sliding_window:if pad_const:→if pad_const is not None:, so a pad constant of0is honored instead of falling through to edge padding.get_window_masks: replacedfor no_data_row in np.where(np.all(~window_masks, axis=1)):with a direct boolean indexwindow_masks[np.all(~window_masks, axis=1)] = True. Same result, no single-iteration loop over anp.wheretuple. Rewrote the docstring to state the actual mask polarity (True= valid) and to explain why all-pad rows are returned fully unmasked.window_min: corrected the docstring, which described a maximum.tests/feature_extraction/window_operations/test_window_stats.py(new)pad_sliding_window(shape, edge padding, constant padding — parametrized over0.0/-1.0/nanso the falsy-constant case is a regression test),get_window_masks(nan and non-nan constants, all-pad rows),window_mean/window_median/window_std_devpadding behavior,window_min/window_maxincluding the all-nan guard, and thatwindow_skew/window_kurtosisagree withnp_skew/np_kurtosison the padded view.Mechanical updates
None — no imports, exports, or call sites were touched.
This PR was produced by an automated analysis from Claude Code.
Generated by Claude Code