Open arsenalgunnershubert777 opened 11 months ago
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 Anomaly
class or the isNewestPointNonAnomalous
method in Check.scala
will see their code fail to compile when they upgrade to this version of Deequ.
Could you update the PR such that the new class is added alongsde Anomaly
? We can also create a new method called isNewestPointNonAnomalousWithExtendedResult
which uses your new class as its return type.
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!
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 isNewestPointAnomalousWithExtendedResult
method. Think of the following 3 scenarios:
@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
@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 AnomalyCheckConfig
available where this new config parameter could be added.
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.