-
Notifications
You must be signed in to change notification settings - Fork 542
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exposing Helpful Anomaly Detection Metadata from Anomaly Strategies (ie Anomaly Thresholds) #525
base: master
Are you sure you want to change the base?
Conversation
…l data points with anomaly detection details, passed that through the constraint into the constraint result as anomaly detection metadata field
…l data points with anomaly detection details, passed that through the constraint into the constraint result as anomaly detection metadata field
Thank you @arsenalgunnershubert777 for the PR and happy new year! We will be reviewing the PR this week. |
@rdsharma26 thanks for the heads up! I am working on fixing the merge conflicts, should be done soon |
@arsenalgunnershubert777 Thanks again for the PR and for your patience while we reviewed this PR. Having looked deeper into the changes, I have one high level comment. This PR introduces changes that are backwards incompatible. Any consumer of Deequ who is using the Could you update the PR such that the new class is added alongsde |
Hi @rdsharma26 thank you for the response. I have some questions for clarity. I will try to make changes to create new class and new method and work for backwards compatibility. But I'm wondering, in the VerificationRunBuilder, in order to make the Anomaly Check use the new feature, I would have to also update this getAnomalyCheck method to use .isNewestPointAnomalousWithExtendedResult right? Would that change affect backwards compatibility as well? Let me know if my understanding of any of this is correct, thanks again! |
Instead of updating, you will add a new method, which then uses the new
|
@rdsharma26 got it, that makes sense! I think I will add an optional parameter in both addAnomalyCheck and getAnomalyCheckMethod to choose which method to use, and it can default to original method. That way everything should be compatible |
Sounds good. Looks like there is |
Hi @rdsharma26 hope you are well, I created new PR to make this backwards compatible thanks! |
Issue #, if available:
521
Description of changes:
This PR adds functionality to expose anomaly detection metadata for anomaly checks
Would love to hear any thoughts, feedback, things to change. Whether relating to overall design, or small renaming and code changes. Thanks so much!
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.