Gerrrr commented on code in PR #169:
URL: https://github.com/apache/otava/pull/169#discussion_r3848900013
##########
tests/series_test.py:
##########
@@ -407,3 +408,83 @@ def test_series_raw_initialization():
assert len(series.time) == 3
assert series.data["throughput"] == [10.0, 12.0, 11.5]
+
+
+def test_change_points_computed_lazily_and_cached(monkeypatch):
+ from otava import series as series_module
+
+ calls = {"count": 0}
+ real_compute = series_module.compute_change_points
+
+ def counting_compute(*args, **kwargs):
+ calls["count"] += 1
+ return real_compute(*args, **kwargs)
+
+ monkeypatch.setattr(series_module, "compute_change_points",
counting_compute)
+
+ data = [1.0] * 10 + [5.0] * 10
+ test = Series(
+ "lazy_test",
+ branch=None,
+ time=list(range(len(data))),
+ metrics={"m1": Metric(1, 1.0), "m2": Metric(1, 1.0)},
+ data={"m1": data, "m2": data.copy()},
+ attributes={},
+ )
+
+ analyzed = test.analyze()
+ assert calls["count"] == 0
+
+ change_points = analyzed.change_points
+ assert calls["count"] == 2 # one computation per metric
+ assert [c.index for c in change_points.get_change_points_for_metric("m1")]
== [10]
+
+ assert analyzed.change_points is change_points
+ assert len(analyzed.change_points_by_time) == 1
+ assert analyzed.weak_change_points is not None
+ assert analyzed.change_points_timestamp is not None
+ assert calls["count"] == 2
+
+
+def test_append_computes_change_points_first():
+ data = [1.0] * 10 + [5.0] * 10
+ test = Series(
+ "append_lazy_test",
+ branch=None,
+ time=list(range(len(data))),
+ metrics={"m1": Metric(1, 1.0)},
+ data={"m1": data},
+ attributes={},
+ )
+
+ analyzed = test.analyze()
+ analyzed.append(time=[len(data)], new_data={"m1": [5.0]}, attributes={})
Review Comment:
This only covers a series with an existing change point. A stable series
computes an empty result, which `_validate_append()` mistakes for “not
computed.” Could you please add a case that does not produce a change point and
explicitly ensure computation rather than checking collection truthiness?
##########
otava/series.py:
##########
@@ -132,29 +132,56 @@ def analyze(self, options: AnalysisOptions =
AnalysisOptions()) -> "AnalyzedSeri
class AnalyzedSeries:
"""
Time series data with computed change points.
+
+ Change points are computed lazily, on first access. Constructing an
Review Comment:
nit: I don't think we need this addition to the doc.
##########
otava/series.py:
##########
@@ -132,29 +132,56 @@ def analyze(self, options: AnalysisOptions =
AnalysisOptions()) -> "AnalyzedSeri
class AnalyzedSeries:
"""
Time series data with computed change points.
+
+ Change points are computed lazily, on first access. Constructing an
+ instance is cheap for callers that never read them.
"""
__series: Series
options: AnalysisOptions
- change_points: ChangePointsByMetric
- change_points_by_time: ChangePointsByTime
- change_points_timestamp: datetime
def __init__(
self, series: Series, options: AnalysisOptions, change_points:
Dict[str, ChangePoint] = None
):
self.__series = series
self.options = options
- # record when these change points were calculated
- self.change_points_timestamp = datetime.now(timezone.utc)
- self.change_points = None
- if change_points is not None:
- self.change_points = change_points
- else:
- cp, weak_cps = self.__compute_change_points(series, options)
- self.change_points = cp
- self.weak_change_points = weak_cps
- self.change_points_by_time =
self.__group_change_points_by_time(series, self.change_points)
+ self.__change_points = change_points
+ self.__weak_change_points = ChangePointsByMetric() if change_points is
not None else None
+ self.__change_points_by_time = None
+ # records when the change points were calculated
+ self.__change_points_timestamp = (
+ datetime.now(timezone.utc) if change_points is not None else None
+ )
+
+ def __ensure_change_points_computed(self):
+ if self.__change_points is None:
+ cp, weak_cps = self.__compute_change_points(self.__series,
self.options)
Review Comment:
`logging.info("Computing change points...` from line 128 should move here.
--
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]
