fix: detect long gradual ramps truncated by false flat detection - #224
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughDerivative-aware flat detection and guarded second-pass padding revise trend boundaries. Core, edge-case, gradual-ramp, and result-format tests now assert updated segment counts and dates. ChangesTrend boundary refinement
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
RussellSB
force-pushed
the
fix/gradual-up-truncated
branch
from
July 12, 2026 08:12
8fe86ea to
78bea84
Compare
Contributor
|
📚 Docs preview removed The docs preview for this PR has been cleaned up. |
The flat detection in process_signals.py used min_nonzero_std (the global minimum non-zero rolling std) as the threshold. During gradual ramps the smoothed signal has low local variation, so smoothed_std drops below this threshold and triggers false flat_flag, fragmenting the contiguous Up. Fix: add a derivative-near-zero check plus an extremely-smooth floor to the flat detection condition. A region is only flagged flat when both smoothed_std is low AND either the derivative is near zero or the signal is so smooth (< 50% of min) that even a positive derivative is noise. This allows genuine gradual ramps (positive derivative, moderate std) to pass through without being misclassified as flat. Reference: issue #195
RussellSB
force-pushed
the
fix/gradual-up-truncated
branch
from
July 12, 2026 08:20
78bea84 to
c34d079
Compare
Guard the pad loop in shave_abrupt_trends with not second_pass so that segments already padded in the first pass are not re-padded during the reclassification second pass. Also clean up redundant padded check in the shave loop.
Replace the not second_pass guard on the pad loop condition with a padded flag check inside the loop. This allows newly reclassified abrupt segments (gradual->abrupt) to be padded in the second pass while still preventing double-padding of segments already padded in the first pass.
…_std The 0.5x threshold was too aggressive - it caused the core gradual test to lose 2 short Flat segments. Increasing to 0.9x preserves those Flat segments while still preventing false flat detection on long gradual ramps (issue #195).
Update segment expectations in 4 tests to match the 0.9x threshold: - test_gradual_ramp_90day_detected: Up start shifted to Apr 29 - test_temp_scenario: Down boundary adjusted - test_noisy_edgecase_1: Noise boundary adjusted - test_noisy_edgecase_4: Up/Flat/Down boundaries adjusted
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
RussellSB
marked this pull request as ready for review
July 13, 2026 06:53
…t at the cost of flat sensitivity on core gradual test case
Adjust segment expectations across 6 test files to match the new 0.835x min_nonzero_std threshold. Core gradual case shifts from 9 to 8 segments (Mar 15-17 Flat absorbed into Up), and related tests update boundary shifts accordingly.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pytrendy/process_signals.py`:
- Around line 185-196: Update the flat-detection logic around rolling_std and
min_nonzero_std so an empty positive-standard-deviation selection defaults
min_nonzero_std to 0.0 instead of NaN. Preserve the existing flat_flag
conditions and ensure both comparisons remain valid for perfectly flat or
shorter-than-WINDOW_FLAT inputs.
In `@tests/tests_crashes_edgecases/test_uncommon_values.py`:
- Around line 148-150: Align the regression test’s implementation and docstring:
in the sustained 90-day uptrend test, either remove the claim about default
parameters or add a separate invocation using default parameters with matching
assertions, while retaining coverage for abrupt_padding=28 with
avoid_noise=False as stated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5790af62-5098-435d-936b-4fa7ef4707d5
⛔ Files ignored due to path filters (2)
tests/tests_crashes_edgecases/data/gradual_ramp_edgecases.csvis excluded by!**/*.csvtests/tests_plotting/core/test_plot_gradual_trends.pngis excluded by!**/*.png
📒 Files selected for processing (9)
pytrendy/post_processing/segments_refine/abrupt_shaving.pypytrendy/process_signals.pytests/test_core_cases.pytests/test_io_results.pytests/tests_crashes_edgecases/data/TESTDATA.mdtests/tests_crashes_edgecases/test_noise_crashes.pytests/tests_crashes_edgecases/test_noise_edgecases.pytests/tests_crashes_edgecases/test_uncommon_values.pytests/tests_noise/test_noise_spikes_gradual.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*: Before pushing, run the core test suite withpytest tests/ -m coreand then run the remaining tests withpytest tests/ -m "not core" --cov-append; CI also uses a 15-second per-test timeout.
Plot regression tests usepytest-mpl, and baselines must be regenerated only againstmatplotlib==3.10.8to avoid false failures.
Files:
tests/tests_crashes_edgecases/data/TESTDATA.mdtests/tests_crashes_edgecases/test_noise_crashes.pytests/tests_noise/test_noise_spikes_gradual.pytests/test_core_cases.pytests/tests_crashes_edgecases/test_uncommon_values.pytests/tests_crashes_edgecases/test_noise_edgecases.pytests/test_io_results.py
pytrendy/**/*
📄 CodeRabbit inference engine (AGENTS.md)
The package implements a single public entrypoint,
pytrendy.detect_trends(df, date_col, value_col, ...), returningPyTrendyResults; changes underpytrendy/should preserve that public API surface.
Files:
pytrendy/post_processing/segments_refine/abrupt_shaving.pypytrendy/process_signals.py
🪛 Ruff (0.15.21)
tests/tests_crashes_edgecases/test_uncommon_values.py
[warning] 163-163: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
🔇 Additional comments (8)
pytrendy/process_signals.py (1)
177-184: LGTM!Also applies to: 197-205
pytrendy/post_processing/segments_refine/abrupt_shaving.py (1)
137-140: LGTM!tests/test_core_cases.py (1)
33-33: LGTM!tests/tests_noise/test_noise_spikes_gradual.py (1)
108-111: LGTM!tests/tests_crashes_edgecases/test_noise_edgecases.py (1)
34-34: LGTM!Also applies to: 51-51, 84-86, 102-103, 206-206
tests/test_io_results.py (1)
246-249: LGTM!Also applies to: 359-359, 390-390, 394-395, 444-448, 480-480, 497-497, 529-529, 546-546, 563-563, 650-651, 661-661, 673-677
tests/tests_crashes_edgecases/data/TESTDATA.md (1)
10-10: LGTM!tests/tests_crashes_edgecases/test_noise_crashes.py (1)
47-47: LGTM!
Owner
Author
|
/oc address coderabitai's recent unresolved comments and resolve |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This was referenced Jul 16, 2026
- Update print_summary() output in README.md and docs/index.md to reflect 8 segments (removed Flat Mar 15-17, extended Up to Mar 17) - Update filter_segments table with new Up segment dates and metrics - Re-execute gradual.ipynb and noise.ipynb with new threshold - Fix noise.ipynb markdown: 9 → 10 noise segments - Run normalize_notebooks.sh for JupyterLite compatibility
…-threshold docs: update outputs, notebooks, and GIFs for 0.835 flat detection threshold
Adds missing fallback logic from plot_pytrendy (lines 80-87, 104-106) that was absent from adjust_segment_boundaries in utils.py.
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
1 participant
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.
Summary
Fixes issue #195: long gradual ramps (e.g. 90-day ramp from 1000→1900) were detected as only ~27 days, with the rest absorbed into Flat.
Also fixes a pre-existing double-padding bug in
abrupt_shaving.pywhere the second shave pass would re-pad already-padded segments.Fix 1: Flat detection during gradual ramps (
process_signals.py)Root cause:
min_nonzero_std(global minimum non-zero rolling std) was too aggressive as the flat threshold. During gradual ramps, the Savitzky-Golay smoothed signal has low local variation, sosmoothed_stddrops below this threshold and triggers falseflat_flag, fragmenting the contiguous Up into short segments.Fix: Add a derivative-near-zero check plus an extremely-smooth floor to the flat detection condition:
smoothed_std <= min_nonzero_std(original condition), AND|deriv| <= derivative_limit), ORsmoothed_std < 83.5% of min_nonzero_std) — so smooth that even a positive derivative is likely noiseThe 83.5% threshold was tuned to preserve the Feb 6-9 Flat in the core gradual test while still preventing false flat detection on long gradual ramps.
Target test:
test_gradual_ramp_90day_detected(new regression test)Fix 2: Double-padding in second shave pass (
abrupt_shaving.py)Root cause: The pad loop in
shave_abrupt_trendsruns on every call, including the second pass after reclassification. This re-pads segments that were already padded in the first pass, extending them by anotherabrupt_paddingdays.Fix: Add a guard inside the pad loop:
if second_pass and segment.get('padded', False): continue. This allows newly reclassified abrupt segments (gradual→abrupt) to be padded while preventing double-padding of already-padded segments.Target test:
test_abrupt_four_spikes(Down total_change preserved at -246.8)Before / After
Before fix — 27-day Up, ramp truncated (from issue #195):
After fix — 83-day Up spanning the ramp (with abrupt_padding=28):
Changes
pytrendy/process_signals.py— flat detection fix:derivative_limitandsmoothed_derivcomputation beforeflat_flagdetectionextremely_smoothcheck (smoothed_std < 83.5% of min_nonzero_std)(derivative_near_zero | extremely_smooth)instead of justderivative_near_zeropytrendy/post_processing/segments_refine/abrupt_shaving.py— double-padding fix:if second_pass and segment.get('padded', False): continueguard inside pad looptests/tests_crashes_edgecases/data/gradual_ramp_edgecases.csv— new test fixture for issue #195tests/tests_crashes_edgecases/test_uncommon_values.py— new regression testtest_gradual_ramp_90day_detectedThreshold tuning analysis
The
extremely_smooththreshold (min_nonzero_std * multiplier) was swept to find the sweet spot where both the core gradual test and the issue #195 test perform well:The 0.835 multiplier preserves the Feb 6-9 Flat while extending the ramp to 83 days (Apr 7 – Jun 29). The Mar 15-17 Flat is lost (Up extends to Mar 17), but this is a reasonable trade-off: the ramp coverage improves by 22 days (61→83d) at the cost of one short Flat segment.
Impact on Gradual Core Test Case
The core gradual test (
test_gradual_trends) now produces 8 segments instead of 9:Before
After
The Mar 15-17 Flat (3 days) is absorbed into the Up segment, extending it by 3 days. This is a positive trade-off: the algorithm is now more robust to minor dips during gradual trends, producing longer and more meaningful segments.
It's not ideal that this core test case is edited as a consequence, however several workarounds were explored and this was deemed to be the best solution with most balanced trade-offs between tests current in test suite. Could be further tuned/expanded on as new scenarios enter the test suite, with the aim of a positive plateau in pure generalisability.
Note: This change affects GIFs, plot baselines, and notebook tutorials that display the gradual column. See #233 for tracking.
Test expectation changes
test_gradual_trendstest_core_cases.pytest_set_summary_structuretest_io_results.pytest_filter_segments_*test_io_results.pytest_integration_full_workflowtest_io_results.pytest_gradual_ramp_90day_detectedtest_uncommon_values.pytest_noisy_edgecase_2test_noise_edgecases.pytest_noisy_edgecase_4test_noise_edgecases.pytest_noisy_edgecase_5test_noise_edgecases.pytest_noisy_edgecase_11test_noise_edgecases.pytest_gradual_spike_single_later_seriestest_noise_spikes_gradual.pyVerification