henrikingo commented on code in PR #154:
URL: https://github.com/apache/otava/pull/154#discussion_r3197403323
##########
otava/series.py:
##########
@@ -259,6 +266,20 @@ def __compute_change_points(
time=series.time[cp_ttest.index], metric=metric,
stats=cp_ttest.stats
)
)
+ elif options.deterministic_edivisive:
+ # weak_change_points == change_points when min_magnitude == 0.
+ # when min_magnitude > 0 then change_points is the subset
where the change was >= min_magnitude.
+ change_points, weak_cps =
compute_change_points_deterministic(values, max_pvalue=options.max_pvalue,
min_magnitude=options.min_magnitude)
+ tester = TTestSignificanceTester(options.max_pvalue)
+ intervals = tester.get_intervals(change_points)
+ for c in change_points:
+ cp_ttest = tester.change_point(c.to_candidate(), values,
intervals)
+ result[metric].append(
+ ChangePoint(
+ index=cp_ttest.index, qhat=cp_ttest.qhat,
+ time=series.time[cp_ttest.index], metric=metric,
stats=cp_ttest.stats
+ )
+ )
Review Comment:
Arguably this is too much copy paste from above and should be refactored
into a separate method. The reason I don't do that is that this transformation
from one ChangePoint to another is the more fundamental problem that we need to
discuss and fix. I've opened #151 for that discussion. In the mean time I
prefer to keep this code ugly and visible so we don't forget to fix it.Trying
to make it look better via refactoring but not solving the fundamental issue is
IMO counter productive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
