Copilot commented on code in PR #154:
URL: https://github.com/apache/otava/pull/154#discussion_r3901340198
##########
otava/series.py:
##########
@@ -194,6 +195,20 @@ def __compute_change_points(
changes={metric: c},
)
result.append(cpg)
+ elif options.deterministic_edivisive:
+ change_points, _ = compute_change_points_deterministic(
+ values,
+ max_pvalue=options.max_pvalue,
+ min_magnitude=options.min_magnitude,
+ )
Review Comment:
Selecting deterministic mode only affects the initial analysis.
`AnalyzedSeries.append()` still unconditionally invokes the split/windowed
`compute_change_points()` path at `otava/series.py:333-340`, so the first
append silently replaces deterministic results for updated metrics with results
from a different algorithm. Since incremental deterministic analysis is
explicitly unsupported, either reject `append()` for this mode or recompute the
full series with the deterministic detector rather than switching algorithms.
##########
otava/analysis.py:
##########
@@ -213,6 +213,44 @@ def compute_change_points_orig(series:
Sequence[SupportsFloat], max_pvalue: floa
return change_points, None
+def compute_change_points_deterministic(series: Sequence[SupportsFloat],
max_pvalue: float = 0.001, min_magnitude: float = 0.0) -> Tuple[PermCPList,
Optional[PermCPList]]:
Review Comment:
The return annotation says these change points contain `PermutationStats`,
but this function constructs them with `TTestStats`. This gives static callers
the wrong public contract and hides T-test-specific fields such as
`tstatistic`. Use `TtestCPList` for both tuple elements.
##########
otava/analysis.py:
##########
@@ -213,6 +213,44 @@ def compute_change_points_orig(series:
Sequence[SupportsFloat], max_pvalue: floa
return change_points, None
+def compute_change_points_deterministic(series: Sequence[SupportsFloat],
max_pvalue: float = 0.001, min_magnitude: float = 0.0) -> Tuple[PermCPList,
Optional[PermCPList]]:
+ """
+ Same as the original algorithm but with deterministic Student T
significance test at the end.
+
+ The motivation for this variation follows from fixing the bug explained at
the top of https://github.com/apache/otava/pull/96
+ The intuition is that the split-merge approach introduced by Datastax is
addressing the same problem that the _kappa_ variable
+ does in the original paper. Now that we compute correctly over all values
of kappa, the split-merge part should be unnecessary,
+ as the original algorithm with kappa bug fixed, will find the same change
points, and more. Therefore the conclusion is we want
+ to go back as much as possible to the original and real algorithm from the
Matteson & James paper. But even then, we find that
+ Student T as significance test is both much faster but also qualitatively
produces better results for the use case we're in at least,
+ that we want to continue using T test and not random permutations for the
significance test.
+
+ TBD: Whether weak change points are still helpful or not. By reading the
problem they fix appears unrelated from the split-merge vs **kappa** symptoms.
+
+ TODO: Support incremental e-divisive. This was easy to implement on top of
the split-merge variation. Not clear what is the correct way here.
+ An easy solution is to rerun from the last change-point, but the problem
is the last change point could itself be influenced by the new data
+ appended.
+ """
+ tester = TTestSignificanceTester(max_pvalue=max_pvalue)
+ detector = ChangePointDetector(significance_tester=tester,
calculator=PairDistanceCalculator)
+ all_change_points = detector.get_change_points(series=series)
Review Comment:
The winning `kappa` is not used by this significance test.
`PairDistanceCalculator` computes `(tau, kappa)` but returns only `tau`
(`otava/change_point_divisive/calculator.py:188-190`), and
`TTestSignificanceTester.get_sides()` therefore compares `series[start:tau]`
with `series[tau:interval.stop]` (`base.py:783-803`). This effectively fixes
`kappa` to the interval end during significance testing, so a temporary change
followed by a reversion can still be rejected—the exact confounding this
variation is intended to avoid. Preserve the winning `kappa` on the candidate
and use `series[tau:kappa]` for this test.
This issue also appears in the following locations of the same file:
- line 236
- line 241
--
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]
