improve(detector): remove &self from OutlierDetector::preprocess #251
Conversation
|
Warning Rate limit exceeded@shenxiangzhuang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request updates the method signatures for the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant D as DbscanDetector
T->>D: Call preprocess(data)
D-->>T: Return processed result
sequenceDiagram
participant T as Test
participant M as MADDetector
T->>M: Call preprocess(data)
Note over M: Compute medians and MAD
M-->>T: Return MAD result
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
🧹 Nitpick comments (1)
crates/augurs-outlier/src/lib.rs (1)
226-226: LGTM! Good architectural improvement.Making
preprocessa static method is a better design choice since preprocessing is a pure function that doesn't require instance state. This change improves API ergonomics by allowing preprocessing without constructing a detector instance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/augurs-outlier/src/dbscan.rs(6 hunks)crates/augurs-outlier/src/lib.rs(2 hunks)crates/augurs-outlier/src/mad.rs(3 hunks)
🔇 Additional comments (3)
crates/augurs-outlier/src/mad.rs (1)
243-250: LGTM! Clean implementation of static preprocessing.The static implementation correctly maintains all functionality while removing unnecessary instance dependency. The use of
Self::for static method calls follows Rust best practices.crates/augurs-outlier/src/dbscan.rs (2)
55-57: LGTM! Consistent implementation of static preprocessing.The implementation correctly follows the trait's new static design while maintaining its simple data transformation logic.
561-561: LGTM! Thorough test updates.All test cases have been consistently updated to use the new static method call pattern.
Also applies to: 597-597, 633-633, 651-651, 660-660
There was a problem hiding this comment.
This is exactly what I had in mind, thanks @shenxiangzhuang! Just one comment on the MAD implementation.
| let medians = Self::calculate_double_medians(y) | ||
| .map_err(|x| PreprocessingError::from(Box::new(x) as Box<dyn std::error::Error>))?; |
There was a problem hiding this comment.
This should use self.medians if it's not None (meaning it's been preset by the user using set_medians), which is what made preprocess_impl a bit more complicated 🙂
There was a problem hiding this comment.
Hmm...I'm a little confused, how could we access self.medians without the &self in fn preprocess(y: &[&[f64]])?
There was a problem hiding this comment.
🤦♂️ good point, I missed that. That's probably why I made it a method rather than an associated function originally 😞
Setting medians is a pretty niche use case but I need to think about whether it's something we should continue to support. I'll have a think and reply soon.
Sorry about this, I missed it completely.
|
I'd hoped this would be possible some other way but the more I look at it the more I think we need to keep the current API for flexibility 😞 for example, we have a use case in MAD which pre-calculates the medians for the last 24 hours and uses those when preprocessing even more recent data, to ensure we're detecting long-term outliers rather than just recent ones. I do think it would be nice to add methods such as Again, sorry I missed this @shenxiangzhuang, and thanks for the contribution! Happy to help/review if you would like to work on the |
|
Never mind! I have learned so much from this project. And sure, I will continue to try to contribute to Augurs in the future! |
Fix #242
Summary by CodeRabbit
DbscanDetectorandMADDetector.