feat: add gradual padding for gradual trends#243
Conversation
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesGradual trend padding
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✨ 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 |
|
📚 Docs preview deployed! Your docs preview is available at: https://russellsb.github.io/pytrendy/pr-243/
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/post_processing/segments_refine/artifact_cleanup.py`:
- Line 262: Update the padded flag evaluation in the segment cleanup logic to
use the dictionary’s `.get()` method directly, removing the explicit `== True`
comparison and redundant membership check while preserving a falsey default when
the key is absent.
In `@pytrendy/post_processing/segments_refine/gradual_expand_contract.py`:
- Line 142: In the segment padding update, simplify the boolean assignment in
the loop around segments_padded by assigning the result of the new_end versus
gradual_end comparison directly, removing the redundant explicit ternary
expression.
- Around line 113-121: Update the gradual padding flow in
gradual_expand_contract to return immediately when segments is empty, before
constructing meta_df or accessing its start and end columns. Preserve the
existing gradual_padding guard and normal processing for non-empty segments.
In `@tests/test_gradual_padding.py`:
- Around line 16-102: In the gradual-padding tests, replace each static
method_params dict() call in test_gradual_padding_28,
test_gradual_padding_28_padded_flags,
test_gradual_padding_168_clamps_to_nonflat, and
test_gradual_padding_zero_no_change with dictionary-literal syntax, preserving
the existing gradual_padding values.
🪄 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: 05f80572-dffe-456d-885e-90db6ea24233
📒 Files selected for processing (5)
pytrendy/detect_trends.pypytrendy/post_processing/segments_refine/__init__.pypytrendy/post_processing/segments_refine/artifact_cleanup.pypytrendy/post_processing/segments_refine/gradual_expand_contract.pytests/test_gradual_padding.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Run Tests
- GitHub Check: Build & Deploy Docs Preview (PR
⚠️ CI failures not shown inline (2)
GitHub Actions: Lint PR Title / Validate PR Title: Feat: add gradual padding for gradual trends
Conclusion: failure
##[group]Run TITLE="Feat: add gradual padding for gradual trends"
�[36;1mTITLE="Feat: add gradual padding for gradual trends"�[0m
�[36;1mPATTERN='^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\(.+\))?!?: .+'�[0m
�[36;1mif ! echo "$TITLE" | grep -qE "$PATTERN"; then�[0m
�[36;1m echo "Error: PR title does not follow Conventional Commits format."�[0m
�[36;1m echo "Title: $TITLE"�[0m
�[36;1m echo "Expected format: <type>(<scope>): <subject>"�[0m
�[36;1m echo "Valid types: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert"�[0m
�[36;1m echo "Examples:"�[0m
�[36;1m echo " feat: add trend detection for seasonal data"�[0m
�[36;1m echo " fix(api): correct calculation in SNR algorithm"�[0m
�[36;1m echo " ci!: remove deprecated workflow"�[0m
�[36;1m exit 1�[0m
�[36;1mfi�[0m
�[36;1mecho "PR title check passed: '$TITLE'"�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
Error: PR title does not follow Conventional Commits format.
Title: Feat: add gradual padding for gradual trends
Expected format: <type>(<scope>): <subject>
Valid types: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert
Examples:
feat: add trend detection for seasonal data
fix(api): correct calculation in SNR algorithm
ci!: remove deprecated workflow
##[error]Process completed with exit code 1.
GitHub Actions: Lint PR Title / 0_Validate PR Title.txt: Feat: add gradual padding for gradual trends
Conclusion: failure
##[group]Run TITLE="Feat: add gradual padding for gradual trends"
�[36;1mTITLE="Feat: add gradual padding for gradual trends"�[0m
�[36;1mPATTERN='^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\(.+\))?!?: .+'�[0m
�[36;1mif ! echo "$TITLE" | grep -qE "$PATTERN"; then�[0m
�[36;1m echo "Error: PR title does not follow Conventional Commits format."�[0m
�[36;1m echo "Title: $TITLE"�[0m
�[36;1m echo "Expected format: <type>(<scope>): <subject>"�[0m
�[36;1m echo "Valid types: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert"�[0m
�[36;1m echo "Examples:"�[0m
�[36;1m echo " feat: add trend detection for seasonal data"�[0m
�[36;1m echo " fix(api): correct calculation in SNR algorithm"�[0m
�[36;1m echo " ci!: remove deprecated workflow"�[0m
�[36;1m exit 1�[0m
�[36;1mfi�[0m
�[36;1mecho "PR title check passed: '$TITLE'"�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
Error: PR title does not follow Conventional Commits format.
Title: Feat: add gradual padding for gradual trends
Expected format: <type>(<scope>): <subject>
Valid types: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert
Examples:
feat: add trend detection for seasonal data
fix(api): correct calculation in SNR algorithm
ci!: remove deprecated workflow
##[error]Process completed with exit code 1.
🧰 Additional context used
📓 Path-based instructions (2)
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/detect_trends.pypytrendy/post_processing/segments_refine/__init__.pypytrendy/post_processing/segments_refine/gradual_expand_contract.pypytrendy/post_processing/segments_refine/artifact_cleanup.py
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/test_gradual_padding.py
🪛 Ruff (0.15.21)
pytrendy/post_processing/segments_refine/gradual_expand_contract.py
[warning] 93-93: Unused function argument: value_col
(ARG001)
[warning] 142-142: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
pytrendy/post_processing/segments_refine/artifact_cleanup.py
[error] 262-262: Avoid equality comparisons to True; use segment['padded']: for truth checks
Replace with segment['padded']
(E712)
tests/test_gradual_padding.py
[warning] 25-25: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
[warning] 46-46: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
[warning] 71-71: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
[warning] 100-100: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
🔇 Additional comments (2)
pytrendy/detect_trends.py (1)
44-77: LGTM!pytrendy/post_processing/segments_refine/__init__.py (1)
9-45: LGTM!
barshadeb
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
RussellSB
left a comment
RussellSB
left a comment
There was a problem hiding this comment.
Hey @barshadeb good job. Left some nitpicky suggestions for maintaining current structures, and advice for how the introduced tests could be more directed.
| segments_refined = group_segments(segments_refined) # grouping 1st pass: sporadic flats & noises | ||
|
|
||
| segments_refined = expand_contract_segments(df, value_col, segments_refined) # for gradual | ||
| segments_refined = pad_gradual_trends(df, value_col, segments_refined, method_params) # for gradual padding |
There was a problem hiding this comment.
I would say not to mud up this high level layer in segments refine.
Follow more of a parallel structure to abrupt - the padded logic is called from within shave_abrupt_trends for example. So follow the same, make the gradual padded logic called from within expand_contract_segments call at the end.
| from copy import deepcopy | ||
| from .trend_classify import classify_trends | ||
| from .gradual_expand_contract import expand_contract_segments | ||
| from .gradual_expand_contract import expand_contract_segments, pad_gradual_trends |
There was a problem hiding this comment.
no need to import once lower comment addressed
| assert_segments_match(results.segments, expected_segments) | ||
|
|
||
| @pytest.mark.core | ||
| def test_gradual_padding_zero_no_change(self): |
There was a problem hiding this comment.
remove this test - it already exists by default as gradual core
| assert_segments_match(results.segments, expected_segments) | ||
|
|
||
| @pytest.mark.core | ||
| def test_gradual_padding_28_padded_flags(self): |
There was a problem hiding this comment.
Remove this test - not entirely important behaviour to focus on to be honest
| assert results.segments[5]['padded'] is True | ||
|
|
||
| @pytest.mark.core | ||
| def test_gradual_padding_168_clamps_to_nonflat(self): |
There was a problem hiding this comment.
Find another example from synthetic or test data to try out this idea on. And you can use segments in a haystack.
The idea is good, it just seems a bit redundant and unvariede to focus on the same input/output as the example above. This scenario in specific doesnt really seem to have enough leeway really for having scenarios where gradual pads all the way up to 168.
Try refer to the example referenced from the issue, one of the recent zero baseline examples for example.
| if 'trend_class' in segment: del segment['trend_class'] | ||
|
|
||
| if trend_ends_too_close or trend_too_small or trend_too_flat: | ||
| if trend_ends_too_close or trend_too_small or (trend_too_flat and not is_padded): |
2 participants
Summary
Adds a
gradual_paddingmethod parameter todetect_trends(), enabling gradual Up/Down trend segments to extend into adjacent flat periods. This mirrors the existingabrupt_paddingbehaviour for gradual trends, allowing users to model sustained effects after a slope change as part of the same treatment period.Closes #112
Motivation
In ITSA and observational causal inference, a gradual slope change may represent the start of a treatment effect, but the actual sustained impact continues into the following flat period. Without padding, these flat periods are classified separately, fragmenting what is logically a single treatment window. Gradual padding lets the trend segment absorb adjacent flat periods so the full extent of the treatment is captured.
Changes
New behaviour
gradual_paddingparameter indetect_trends()(default0, no change to existing behaviour)padded: Trueflag to distinguish them from unpadded segmentsFiles changed
pytrendy/detect_trends.pygradual_paddinginmethod_paramspytrendy/post_processing/segments_refine/__init__.pypad_gradual_trendsafter expand/contract, beforeabrupt_shavingpytrendy/post_processing/segments_refine/gradual_expand_contract.pypad_gradual_trends()function with overlap avoidance, dataset-end clamping, and padded flagpytrendy/post_processing/segments_refine/artifact_cleanup.pypaddedflag to prevent reclassification of padded segmentstests/test_gradual_padding.pyTests added
test_gradual_padding_28test_gradual_padding_28_padded_flagspadded=Trueset only on segments that were actually extendedtest_gradual_padding_168_clamps_to_nonflattest_gradual_padding_zero_no_changegradual_padding=0(default) produces identical results to no padding📈 View Segment Transition Summary (Before vs. After)
Before (no padding) — 9 segments:
Up(2025-01-02 → 2025-01-24)Down(2025-01-25 → 2025-02-05)Flat(2025-02-06 → 2025-02-09)Up(2025-02-10 → 2025-03-14)Flat(2025-03-15 → 2025-03-17)Down(2025-03-18 → 2025-04-01)Up(2025-04-02 → 2025-05-08)Down(2025-05-09 → 2025-06-17)Flat(2025-06-18 → 2025-06-30)After (gradual_padding=28) — 6 segments, 0 flats:
Up(2025-01-02 → 2025-01-24) — no room to padDown(2025-01-25 → 2025-02-09) — padded into flat ✓Up(2025-02-10 → 2025-03-17) — padded into flat ✓Down(2025-03-18 → 2025-04-01) — no room to padUp(2025-04-02 → 2025-05-08) — no room to padDown(2025-05-09 → 2025-06-30) — padded to dataset end ✓The key difference: The 3
Flatgaps (Feb 6–9, Mar 15–17, and Jun 18–30) are successfully absorbed into adjacent trend segments, resulting in 6 continuous trend periods instead of 9 fragmented ones.