change: use u32 instead of i32 for lower/upper windows #177
Conversation
The Python Prophet library expects the lower window to be negative and the upper window to be positive, but here it makes more sense to restrict both to being positive and just assume that the lower window refers to 'before the holiday date'.
Deploying augurs with
|
| Latest commit: |
157eb4a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bdea195d.augurs.pages.dev |
| Branch Preview URL: | https://holiday-lower-upper-u32.augurs.pages.dev |
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
crates/augurs-prophet/src/features.rs (3)
20-21: Document the deviation from Python Prophet's conventionWhile changing to
u32makes the API more intuitive (interpreting windows as time periods before/after holiday), this deviates from Python Prophet's convention of using negative values for lower windows. Consider adding a note in the struct documentation explaining this design choice and how it differs from Python Prophet./// A holiday to be considered by the Prophet model. +/// +/// Note: Unlike Python Prophet which uses negative values for lower windows, +/// this implementation uses positive values to represent time periods before +/// the holiday date. For example, a lower window of 2 means the holiday +/// effect starts 2 days before the actual holiday date. #[derive(Debug, Clone)] pub struct Holiday {
40-42: Enhance documentation with valid range informationThe documentation example is clear, but it would be helpful to explicitly state that the window values must be non-negative integers.
/// Set the lower window for the holiday. /// /// The lower window is the number of days before the holiday /// that it is observed. For example, if the holiday is on /// 2023-01-01 and the lower window is 1, then the holiday will - /// _also_ be observed on 2022-12-31. + /// _also_ be observed on 2022-12-31. + /// + /// Note: Window values must be non-negative integers, representing + /// the number of days before the holiday.
61-61: Add non-negative range information to upper_window documentationFor consistency with the lower_window documentation, consider adding a note about the non-negative requirement.
/// Set the upper window for the holiday. /// /// The upper window is the number of days after the holiday /// that it is observed. For example, if the holiday is on /// 2023-01-01 and the upper window is 1, then the holiday will - /// _also_ be observed on 2023-01-02. + /// _also_ be observed on 2023-01-02. + /// + /// Note: Window values must be non-negative integers, representing + /// the number of days after the holiday.js/augurs-prophet-js/src/lib.rs (1)
1233-1236: Consider clarifying the behavior change in the documentation.While the documentation explains how the lower window works with positive values, it would be helpful to explicitly mention that this is a departure from Python Prophet's behavior where lower windows are traditionally negative values. This change in behavior should be prominently documented to avoid confusion for users migrating from Python Prophet.
Consider adding a note like:
/// The lower window for the holiday. /// /// The lower window is the number of days before the holiday /// that it is observed. For example, if the holiday is on /// 2023-01-01 and the lower window is 1, then the holiday will /// _also_ be observed on 2022-12-31. + /// + /// Note: Unlike Python Prophet which uses negative values for lower windows, + /// this implementation uses positive values to represent days before the holiday.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
crates/augurs-prophet/src/features.rs(3 hunks)crates/augurs-prophet/src/prophet/prep.rs(2 hunks)js/augurs-prophet-js/src/lib.rs(2 hunks)
🔇 Additional comments (6)
crates/augurs-prophet/src/features.rs (1)
20-21: Verify the impact of type changes
The change from i32 to u32 for window values is a breaking change that needs careful verification.
Also applies to: 42-42, 61-61
✅ Verification successful
Let me gather more information about the type changes and their impact.
Let me also check the test files specifically:
Based on the gathered information, I can now provide a final response:
Type change from i32 to u32 appears safe to proceed
The change from i32 to u32 for window values is actually safe because:
- All existing usage patterns in the codebase only use non-negative values (e.g.,
vec![0, 0],vec![1, 1]) - The fields are only accessed through builder methods
with_lower_windowandwith_upper_window - The JavaScript bindings in
augurs-prophet-jshandle the conversion appropriately - No test cases were found using negative values for these fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes and usage patterns
# Check for direct usage of Holiday struct fields
echo "Checking direct field access patterns..."
rg -A 2 "holiday\.(lower|upper)_window"
# Check for calls to with_lower_window and with_upper_window methods
echo "Checking method call patterns..."
rg -A 2 "with_(lower|upper)_window\("
# Look for test files that might need updates
echo "Checking test files..."
fd -e rs test | xargs rg -A 2 "Holiday"
Length of output: 2159
Script:
#!/bin/bash
# Check the git history for these fields to understand the type change
git log -L :lower_window:crates/augurs-prophet/src/features.rs -L :upper_window:crates/augurs-prophet/src/features.rs
# Look for any negative values being used in tests or examples
rg -A 2 "(-[0-9]+|[0-9]+)" crates/augurs-prophet/src/features.rs
# Check for any error handling or validation related to these fields
ast-grep --pattern 'fn with_lower_window($$$) { $$$ }'
ast-grep --pattern 'fn with_upper_window($$$) { $$$ }'
Length of output: 1443
Script:
# Check test files for window-related tests
fd -e rs test | xargs rg "(lower|upper)_window"
# Look for any validation or conversion of window values in tests
fd -e rs test | xargs rg "Holiday.*window"
Length of output: 94
js/augurs-prophet-js/src/lib.rs (2)
1245-1245: LGTM: Type change aligns with the design decision.
The change from Option<Vec<i32>> to Option<Vec<u32>> for upper_window is consistent with the overall design decision to use unsigned integers for window values.
Line range hint 1233-1245: Verify handling of unsigned windows across the codebase.
Let's ensure that all code handling these windows has been updated to work with unsigned integers.
✅ Verification successful
All window-related code consistently uses unsigned integers (u32)
Based on the search results:
- All occurrences of
lower_windowandupper_windowin the codebase useVec<u32>type - No instances of signed integer (
i32) usage with windows were found - The implementation is well documented with references to Python/R Prophet compatibility
- The conversion functions
with_lower_windowandwith_upper_windowproperly handle the unsigned values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining signed integer handling of window values
# and verify documentation of the behavioral change.
# Check for any remaining i32 types used with windows
rg -g '*.rs' '(?i)(lower|upper)_window.*i32'
# Look for conversion or handling of window values
rg -g '*.rs' '(?i)(lower|upper)_window'
# Check for documentation about Python/R Prophet compatibility
rg -g '*.{rs,md}' -i 'python.*prophet|prophet.*python'
Length of output: 4545
crates/augurs-prophet/src/prophet/prep.rs (3)
667-669: LGTM: Safe type conversion for lower window values.
The conversion from u32 to i32 is safe and necessary for later negation of the lower window values.
674-676: LGTM: Safe type conversion for upper window values.
The conversion from u32 to i32 is safe and maintains consistency with the lower window conversion.
685-687: LGTM: Correctly handles negative offsets while using u32.
The implementation elegantly solves the requirement to use u32 for window values while maintaining the expected behavior of negative offsets for the lower window. The comment clearly explains the rationale.
The Python Prophet library expects the lower window to be negative
and the upper window to be positive, but here it makes more sense
to restrict both to being positive and just assume that the lower
window refers to 'before the holiday date'.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation