henrikingo commented on code in PR #161:
URL: https://github.com/apache/otava/pull/161#discussion_r3492097220
##########
otava/change_point_divisive/base.py:
##########
@@ -14,61 +14,640 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+"""
+Hierarchy of ChangePoint classes:
+ CandidateChangePoint <--> ChangePoint --> ChangePointSerializer
+ .index .index .to_json()
+ , .stats .get_this_or_that()
+ ^
+ / `------BaseStats
+ ^ .pvalue
+ / `
+ `GenericStats
+ / TTestStats
+ PermutationStats
+ /
+ ChangePointGroup
+ .time
+ .attributes.commit
+ .changes[metric, ChangePoint]
+ # Essentially a row: One or more ChangePoint at the same
commit/time
+
+ /
+ ChangePoints
+ # Typically all change points for a given test / run / etc
+ ^
+ |
+ ^
+ ChangePointsByTime `ChangePointsByMetric
+ .change_points: list(ChangePointGroup) .change_points:
dict[metric, list(ChangePointGroup)]
+ .pivot() < - - > .pivot()
+"""
+
+from collections import OrderedDict
from dataclasses import dataclass, fields
-from typing import Generic, List, Optional, TypeVar
+from datetime import UTC, datetime
+from typing import Dict, Generic, List, Optional, TypeVar, Sequence,
SupportsFloat, Any
+import numpy as np
from numpy.typing import NDArray
@dataclass
class CandidateChangePoint:
- '''Candidate for a change point. The point that maximizes Q-hat function
on [start:end+1] slice'''
+ """Candidate for a change point. The point that maximizes Q-hat function
on [start:end+1] slice"""
+
index: int
qhat: float
@dataclass
class BaseStats:
- '''Abstract statistics class for change point. Implementation depends on
the statistical test.'''
+ """Abstract statistics class for change point. Implementation depends on
the statistical test."""
+
+ # The pvalue for this change point. Exact value depends on the algorithm
that was used.
pvalue: float
+ mean_1: float
+ mean_2: float
+ std_1: float
+ std_2: float
+
+ def __init__(self, left: Sequence[SupportsFloat], right:
Sequence[SupportsFloat], pvalue=None) -> Any:
+ """
+ Basic statsistics about the left and right side, and the change
between them.
+
+ Calculate basic statistics about the left and right sides of a change
point, such as mean
+ standard deviation. p-value depeds on the significance test used, so
we cannot know or compute
+ it here, but if the caller knows p already, they can supply it as
argument.
+ """
+ self.calculate_base_stats(left, right, pvalue)
+
+ def calculate_base_stats(self, left, right, pvalue=None):
+ if pvalue is not None and pvalue >= 0.0 and pvalue <= 1.0:
+ self.pvalue = pvalue
+ else:
+ self.pvalue = 1.0
+
+ if len(left) == 0 or len(right) == 0:
+ raise ValueError
+
+ self.mean_1 = np.mean(left)
+ self.mean_2 = np.mean(right)
+ self.std_1 = np.std(left) if len(left) >= 2 else 0.0
+ self.std_2 = np.std(right) if len(right) >= 2 else 0.0
+
+ return self
+
+ def forward_rel_change(self, value_if_nan=0):
+ """Relative change from left to right"""
+ if self.mean_1 == 0:
+ return value_if_nan
+
+ return self.mean_2 / self.mean_1 - 1.0
+
+ def backward_rel_change(self, value_if_nan=0):
+ """Relative change from right to left"""
+ if self.mean_2 == 0:
+ return value_if_nan
+
+ return self.mean_1 / self.mean_2 - 1.0
+ def forward_change_percent(self) -> float:
+ return self.forward_rel_change() * 100.0
+
+ def backward_change_percent(self) -> float:
+ return self.backward_rel_change() * 100.0
+
+ def change_magnitude(self):
+ """Maximum of absolutes of rel_change and rel_change_reversed"""
+ return max(abs(self.forward_rel_change()),
abs(self.backward_rel_change()))
+
+ def mean_before(self):
+ return self.mean_1
+
+ def mean_after(self):
+ return self.mean_2
+
+ def stddev_before(self):
+ return self.std_1
+
+ def stddev_after(self):
+ return self.std_2
+
+ def to_json(self):
+ return {
+ "forward_change_percent": f"{self.forward_change_percent():-0f}",
+ "magnitude": f"{self.change_magnitude():-0f}",
+ "mean_before": f"{self.mean_before():-0f}",
+ "stddev_before": f"{self.stddev_before():-0f}",
+ "mean_after": f"{self.mean_after():-0f}",
+ "stddev_after": f"{self.stddev_after():-0f}",
+ "pvalue": f"{self.pvalue:-0f}",
+ }
# Abstract variable type for statistics, corresponds to BaseStats class and
its subclasses.
GenericStats = TypeVar("GenericStats", bound=BaseStats)
@dataclass
class ChangePoint(CandidateChangePoint, Generic[GenericStats]):
- '''Change point class, defined by index and signigicance test statistic.'''
+ """
+ ChangePoint class.
+
+ Defined by index and signigicance test statistic.
+ This class is the basic change point that is used during computation
+ and returned as a result. This class does not however carry additional
+ attributes like metric, time, or commit sha. Those are in ChangePointGroup
+ and ChangePoints.
+ Note that while in theory the index, commit sha, an the time(stamp) should
+ all be the same, in practice they aren't always. For example if at some
point
+ during a tests lifetime, more metrics are added to the output, then
different
+ metrics will have different histories and therefore their indexes start
from
+ different locations.
+ To use time(stamp), metric name or timestamp, to access change points,
please
+ use the ChangePointGroup and ChangePoints classes.
+ """
+
stats: GenericStats
+ # Which metric this change point belongs to. (This is redundant and for
convenience.)
+ metric: Optional[str] = None
def __eq__(self, other):
- '''Helpful to identify new Change Points during divisive algorithm'''
+ """Helpful to identify new Change Points during divisive algorithm"""
return isinstance(other, self.__class__) and self.index == other.index
@classmethod
- def from_candidate(cls, candidate: CandidateChangePoint, stats:
GenericStats) -> 'ChangePoint[GenericStats]':
+ def from_candidate(
+ cls, candidate: CandidateChangePoint, stats: GenericStats
+ ) -> "ChangePoint[GenericStats]":
return cls(
index=candidate.index,
qhat=candidate.qhat,
stats=stats,
)
def to_candidate(self) -> CandidateChangePoint:
- '''Downgrades Change Point to a Candidate Change Point. Used to
recompute stats for Weak Change Points.'''
+ """Downgrades Change Point to a Candidate Change Point. Used to
recompute stats for Weak Change Points."""
data = {f.name: getattr(self, f.name) for f in
fields(CandidateChangePoint)}
return CandidateChangePoint(**data)
+ def to_json(self, rounded=True):
+ cps = ChangePointSerializer(self)
+ return cps.to_json(rounded)
+
+
+class ChangePointSerializer(ChangePoint):
+ """
+ Utility class with getters and json serialization for a ChangePoint.
+
+ TODO: Maintaining this is tedious. We should replace it with pydantic or
some
Review Comment:
I'll create a follow up issue, at least provided that fixing the bugs
already happening here isn't more work than migrating.
##########
otava/series.py:
##########
@@ -445,12 +365,12 @@ def metric(self, name: str) -> Metric:
def to_json(self):
change_points_json = {}
- for metric, cps in self.change_points.items():
- change_points_json[metric] = [cp.to_json(rounded=False) for cp in
cps]
+ for cps in self.change_points:
+ change_points_json = [cp.to_json(rounded=False) for cp in cps]
Review Comment:
punting on the test as I don't expect these custom to_json/from_json to
survive the 0.8.x series...
##########
otava/bigquery.py:
##########
@@ -87,9 +87,10 @@ def insert_change_point(
test: BigQueryTestConfig,
metric_name: str,
attributes: Dict,
- change_point: ChangePoint,
+ change_point_group: ChangePointGroup,
):
- kwargs = {**attributes, **{test.time_column:
datetime.utcfromtimestamp(change_point.time)}}
+ change_point = change_point_group[metric_name]
Review Comment:
...and the reason this is untested because bigquery is a cloud-only database
so can't test it without internet?
##########
tests/series_test.py:
##########
@@ -90,7 +145,7 @@ def test_div_by_zero():
cpjson = analyzed_series.to_json()
assert cpjson
assert len(change_points) == 2
- assert change_points[0].index == 3
Review Comment:
I think this particular test is testing something rather specific (given its
name) so I won't add generic assertions here
##########
otava/series.py:
##########
@@ -445,12 +365,12 @@ def metric(self, name: str) -> Metric:
def to_json(self):
change_points_json = {}
- for metric, cps in self.change_points.items():
- change_points_json[metric] = [cp.to_json(rounded=False) for cp in
cps]
+ for cps in self.change_points:
+ change_points_json = [cp.to_json(rounded=False) for cp in cps]
Review Comment:
You're right about the ChangePointGroups not serializing correctly. The
glass half full version of this story is that it was easy to fix with the
current ChangePoints* class and hierarcy of data structures.
Clearly the intent has been to do something that follows the by_metric()
structure, so I fixed it based on that.
As for the latter half of your comment, the serialization doesn't lose data,
rather timestamps and attributes are part of the test results data (Series) . A
change point only has the minimal info about the change itself, and reference
back to the overall Series, such as metric and index.
##########
otava/series.py:
##########
@@ -216,14 +137,17 @@ class AnalyzedSeries:
__series: Series
options: AnalysisOptions
- change_points: Dict[str, List[ChangePoint]]
- change_points_by_time: List[ChangePointGroup]
- change_points_timestamp: Any
+ change_points: ChangePointsByMetric
+ change_points_by_time: ChangePointsByTime
+ change_points_timestamp: ChangePointsByMetric
Review Comment:
Actually it should be datetime. (See 5 lines below) Thanks!
##########
otava/change_point_divisive/base.py:
##########
@@ -40,39 +155,619 @@ class BaseStats:
@dataclass
class ChangePoint(CandidateChangePoint, Generic[GenericStats]):
- '''Change point class, defined by index and signigicance test statistic.'''
+ """
+ ChangePoint class.
+
+ Defined by index and signigicance test statistic.
+ This class is the basic change point that is used during computation
+ and returned as a result. This class does not however carry additional
+ attributes like metric, time, or commit sha. Those are in ChangePointGroup
+ and ChangePoints.
+ Note that while in theory the index, commit sha, an the time(stamp) should
+ all be the same, in practice they aren't always. For example if at some
point
+ during a tests lifetime, more metrics are added to the output, then
different
+ metrics will have different histories and therefore their indexes start
from
+ different locations.
+ To use time(stamp), metric name or timestamp, to access change points,
please
+ use the ChangePointGroup and ChangePoints classes.
+ """
+
stats: GenericStats
+ # Which metric this change point belongs to. (This is redundant and for
convenience.)
+ metric: Optional[str] = None
+
+ def copy(self):
+ """
+ Copy constructor.
+
+ :return: A deep copy of self, recursively calls also stats.copy().
+ """
+ return ChangePoint(
+ index=self.index, qhat=self.qhat, stats=self.stats.copy(),
metric=self.metric
+ )
def __eq__(self, other):
- '''Helpful to identify new Change Points during divisive algorithm'''
+ """Helpful to identify new Change Points during divisive algorithm"""
return isinstance(other, self.__class__) and self.index == other.index
Review Comment:
It most certainly isn't in the general case and even here one should at
least require `other.metric == self.metric`
I poisoned this with an assert False and turns out there were only 2 places
that used this. Changed them to explicitly check for .index and then removed
this __eq__ method.
##########
otava/change_point_divisive/base.py:
##########
@@ -40,39 +155,619 @@ class BaseStats:
@dataclass
class ChangePoint(CandidateChangePoint, Generic[GenericStats]):
- '''Change point class, defined by index and signigicance test statistic.'''
+ """
+ ChangePoint class.
+
+ Defined by index and signigicance test statistic.
+ This class is the basic change point that is used during computation
+ and returned as a result. This class does not however carry additional
+ attributes like metric, time, or commit sha. Those are in ChangePointGroup
+ and ChangePoints.
+ Note that while in theory the index, commit sha, an the time(stamp) should
+ all be the same, in practice they aren't always. For example if at some
point
+ during a tests lifetime, more metrics are added to the output, then
different
+ metrics will have different histories and therefore their indexes start
from
+ different locations.
+ To use time(stamp), metric name or timestamp, to access change points,
please
+ use the ChangePointGroup and ChangePoints classes.
+ """
+
stats: GenericStats
+ # Which metric this change point belongs to. (This is redundant and for
convenience.)
+ metric: Optional[str] = None
+
+ def copy(self):
+ """
+ Copy constructor.
+
+ :return: A deep copy of self, recursively calls also stats.copy().
+ """
+ return ChangePoint(
+ index=self.index, qhat=self.qhat, stats=self.stats.copy(),
metric=self.metric
+ )
def __eq__(self, other):
- '''Helpful to identify new Change Points during divisive algorithm'''
+ """Helpful to identify new Change Points during divisive algorithm"""
return isinstance(other, self.__class__) and self.index == other.index
@classmethod
- def from_candidate(cls, candidate: CandidateChangePoint, stats:
GenericStats) -> 'ChangePoint[GenericStats]':
+ def from_candidate(
+ cls, candidate: CandidateChangePoint, stats: GenericStats
+ ) -> "ChangePoint[GenericStats]":
return cls(
index=candidate.index,
qhat=candidate.qhat,
stats=stats,
)
def to_candidate(self) -> CandidateChangePoint:
- '''Downgrades Change Point to a Candidate Change Point. Used to
recompute stats for Weak Change Points.'''
+ """Downgrades Change Point to a Candidate Change Point. Used to
recompute stats for Weak Change Points."""
data = {f.name: getattr(self, f.name) for f in
fields(CandidateChangePoint)}
return CandidateChangePoint(**data)
+ def to_json(self, rounded=True):
+ cps = ChangePointSerializer(self)
+ return cps.to_json(rounded)
+
+
+class ChangePointSerializer(ChangePoint):
+ """
+ Utility class with getters and json serialization for a ChangePoint.
+
+ TODO: Maintaining this is tedious. We should replace it with pydantic or
some
+ other standard solution that provides json serialization.
+ """
+
+ def __init__(self, cp: ChangePoint[GenericStats]):
+ self.stats = cp.stats
+ self.index = cp.index
+ self.metric = cp.metric
+
+ def forward_change_percent(self) -> float:
+ return self.stats.forward_rel_change() * 100.0
+
+ def backward_change_percent(self) -> float:
+ return self.stats.backward_rel_change() * 100.0
+
+ def magnitude(self):
+ return self.stats.change_magnitude()
+
+ def mean_before(self):
+ return self.stats.mean_1
+
+ def mean_after(self):
+ return self.stats.mean_2
+
+ def stddev_before(self):
+ return self.stats.std_1
+
+ def stddev_after(self):
+ return self.stats.std_2
+
+ def pvalue(self):
+ return self.stats.pvalue
+
+ def to_json(self, rounded=True):
+ if rounded:
+ return {
+ "metric": self.metric,
+ "index": int(self.index),
+ "forward_change_percent":
f"{self.forward_change_percent():.0f}",
+ "backward_change_percent":
f"{self.backward_change_percent():.0f}",
+ "magnitude": f"{self.magnitude():-0f}",
+ "mean_before": f"{self.mean_before():-0f}",
+ "stddev_before": f"{self.stddev_before():-0f}",
+ "mean_after": f"{self.mean_after():-0f}",
+ "stddev_after": f"{self.stddev_after():-0f}",
+ "pvalue": f"{self.pvalue():-0f}",
+ }
+
+ else:
+ return {
+ "metric": self.metric,
+ "index": int(self.index),
+ "forward_change_percent": self.forward_change_percent(),
+ "magnitude": self.magnitude(),
+ "mean_before": self.mean_before(),
+ "stddev_before": self.stddev_before(),
+ "mean_after": self.mean_after(),
+ "stddev_after": self.stddev_after(),
+ "pvalue": self.pvalue(),
+ }
+
+
+@dataclass
+class ChangePointGroup:
+ """
+ A group of change points on multiple metrics, at the same point in time.
+
+ If you think of ChangePoints as a 2D table, then ChangePointGroup are
rows, and the metrics are the columns.
+ One ChangePointGroup has only the metrics where a ChangePoint was found at
this time(stamp).
+ Note that there is no .index at this level. This is because each metric is
an independent sequence of results,
+ and each such sequence may have started at different times in the past,
and may also be missing observations
+ where others do have some. Because of this, each ChangePoint has its own
.index in
+ ChangePointGroup.changes[metric_name].index
+
+ :param time: The unix timestamp for the results that ChangePoint(s) were
found at. Decimals are milliseconds.
+ :param attributes: The attributes of the test result at this timestamp.
Commit and test metadata.
+ :param changes: For each metric that has a change point at this
time(stamp), the ChangePoint object.
+ """
+ time: float
+ attributes: Dict[str, str]
+ # ChangePointGroup.changes.keys() stores the set of metrics that were used
at this ChangePointGroup.time.
+ changes: Dict[str, ChangePoint]
+
+ def to_json(self, rounded=False):
+ changes = []
+ for metric, cp in self.changes.items():
+ changes.append(cp.to_json(rounded=rounded))
+
+ return {
+ "time": self.time,
+ "attributes": self.attributes,
+ "changes": changes,
+ }
+
+ def copy(self):
+ new_attributes = {k: v for k, v in self.attributes.items()}
+ new_changes = {metric: cp.copy() for metric, cp in
self.changes.items()}
+ new_obj = ChangePointGroup(time=self.time, attributes=new_attributes,
changes=new_changes)
+ return new_obj
+
+ def __getitem__(self, metric):
+ return self.changes[metric]
+
+ def metrics(self):
+ return self.changes.keys()
+
+ def commit(self):
+ return self.attributes.get("commit")
+
+ def datetime(self):
+ return datetime.fromtimestamp(self.time, timezone.utc)
+
+ def select_metrics(self, m: list[str] | str):
+ if not isinstance(m, list):
+ m = [m]
+ filtered = ChangePointGroup(time=self.time,
attributes=self.attributes, changes={})
+ for metric, cp in self.changes.items():
+ if metric in m:
+ filtered.changes[metric] = cp
+ return filtered
+
+ def set(self, metric: str, cp: ChangePoint):
+ self.changes[metric] = cp
+
+ def __iter__(self):
+ return iter([v for v in list(self.changes.values())])
+
+
+class ChangePoints:
+ """
+ A list of ChangePointGroup objects.
+
+ Typical usage of this would be to hold all the change points over a
history of a single test,
+ the test producing one or more metrics. Note that this is a sparse
structure. It only
+ holds ChangePoint objects at the time and metric (row and column...) that
one was found.
+
+ Specifically: it is NOT guaranteed that each row (each GhangePointGroup)
has each metric.
+
+ Companion class ChangePointsByMetric is expected to provide functionally
equivalent interface, but
+ storing each series separately by metric, which is used in parts of the
code base, in particular, what
+ Series.analyze() returns.
+ Subclass ChangePointsByTime is this same class, but can be used if you
explicitly want to mark the ordering used.
+ """
+
+ def __init__(self):
+ """
+ The constructor only creates an empty container. To build one from
+ existing data, use the explicit from_list() / from_dict() classmethods.
+ """
+ self._change_points = []
+
+ @classmethod
+ def from_list(cls, cps: list[ChangePointGroup]):
+ """Build from a list of ChangePointGroup objects, ordered by time."""
+ if not isinstance(cps, list):
+ raise TypeError(
+ f"from_list() argument must be a list. Got {type(cps)}."
+ )
+ for cpg in cps:
+ if not isinstance(cpg, ChangePointGroup):
+ raise TypeError(
+ f"from_list() takes a list of ChangePointGroup objects.
Got {type(cpg)}."
+ )
+ obj = cls()
+ for cpg in sorted(cps, key=lambda cpg: cpg.time):
+ obj.append(cpg)
+ return obj
+
+ @classmethod
+ def from_dict(cls, cps: dict):
+ """
+ Build a ChangePointsByMetric from a dict[str, list[ChangePointGroup]].
+
+ A dict is inherently keyed by metric, so from_dict() returns a
+ ChangePointsByMetric. (This is asymmetric with from_list(), which
returns
+ whichever class you call it on.) ChangePointsByTime overrides this to
reject
+ a dict outright.
+ """
+ if not isinstance(cps, dict):
+ raise TypeError(
+ f"from_dict() takes a dict[str, list[ChangePointGroup]]. Got
{type(cps)}."
+ )
+ obj = ChangePointsByMetric()
+ for metric, cpglist in cps.items():
+ for cpg in cpglist:
+ if not isinstance(cpg, ChangePointGroup):
+ raise TypeError(
+ "from_dict() takes a dict[str,
list[ChangePointGroup]]."
+ )
+ for cp in cpg:
+ if cp.metric and cp.metric != metric:
+ raise ValueError(f"metric field is not internally
consistent. {cp.metric} != {metric} at {cpg.time}")
+ # store each metric's groups sorted by timestamp
+ obj._change_points[metric] = sorted(cpglist, key=lambda cpg:
cpg.time)
+ return obj
+
+ def copy(self):
+ """
+ Copy constructor.
+
+ Returns a deep copy of this object.
+ """
+ new_obj = ChangePointsByTime()
+ new_obj._change_points = [
+ cpg.copy() for cpg in sorted(self._change_points, key=lambda cpg:
cpg.time)
+ ]
+ return new_obj
+
+ def by_time(self):
+ return self
+
+ def by_metric(self):
+ return self.pivot()
+
+ def append(self, cpg: ChangePointGroup):
+ if not isinstance(cpg, ChangePointGroup):
+ raise TypeError("ChangePoints.append() takes as argument one
ChangePointGroup.")
+
+ if (not self._change_points) or cpg.time >
self._change_points[-1].time:
+ self._change_points.append(cpg)
+ elif self._change_points and cpg.time == self._change_points[-1].time:
+ for metric, cp in cpg.changes.items():
+ if metric in self._change_points[-1].changes:
+ raise KeyError("Duplicate keys. Shouldn't happen.")
+ self._change_points[-1].changes[metric] = cp
+ else:
+ # TODO: logging
+ # print(self._change_points)
+ # print(cpg)
+ raise ValueError(
+ "ChangePoints.append() can only be used such that time is
monotonically increasing"
+ )
+
+ def extend(self, cps: list[ChangePointGroup]):
+ errmsg = "ChangePoints.extend() takes as argument a list of
ChangePointGroup objects."
+ if not isinstance(cps, list):
+ raise TypeError(errmsg)
+ for obj in cps:
+ if not isinstance(obj, ChangePointGroup):
+ raise TypeError(errmsg)
+ if (not self._change_points) or obj.time >
self._change_points[-1].time:
+ self._change_points.append(obj)
+ else:
+ raise ValueError(
+ "ChangePoints.extend() can only be used such that time is
monotonically increasing"
+ )
+
+ def items(self):
+ return self.pivot().items()
+
+ def __iter__(self):
+ return iter(self._change_points)
+
+ def __len__(self):
+ return len(self._change_points)
+
+ def __getitem__(self, n):
+ return self._change_points[n]
+
+ def __contains__(self, metric):
+ """
+ True if this container holds at least one ChangePoint where
metric==metric
+
+ This makes more sense, and is more efficient, for ChangePointsByMetric
class, but we provide
+ the same functionality here for consistency.
+ """
+ return metric in self.metrics()
+
+ def metrics(self) -> set:
+ all_metrics = set()
+ for row in self._change_points:
+ all_metrics.update(row.metrics())
+ return all_metrics
+
+ def select_metrics(self, m: list[str] | str):
+ """
+ Get a new ChangePoints object holding only the given metric(s).
+
+ If you think of a ChangePoints object as timestamps being rows, and
+ the metrics being columns, then this returns a subset of the columns.
+
+ Note: The internal data structure doesn't do anything to make this
+ request efficient. This will loop over all ChangePointGroup s.
+ Use ChangePointsByMetric if you need this to be fast.
+ """
+ filtered = ChangePoints()
+ for cpg in self._change_points:
+ filtered.append(cpg.select_metrics(m))
+ return filtered
+
+ def get_change_points_for_metric(self, m: str):
+ single_metric = self.select_metrics(m)
+ metric_change_points = []
+ for cpg in single_metric._change_points:
+ for metric, cp in cpg.changes.items():
+ if cp.metric and cp.metric != metric:
+ raise ValueError(f"metric field is not internally
consistent. {cp.metric} != {metric} at {cpg.time}")
+ metric_change_points.append(cp)
+ return metric_change_points
+
+ def at_timestamp(self, t: float):
+ for cpg in self._change_points:
+ if cpg.time == t:
+ return cpg
+ if abs(cpg.time - t) < 0.0001:
+ return cpg
+ raise LookupError(t)
+
+ def at_commit(self, sha: str):
+ for row in self:
+ if row.attributes['commit'] == sha:
+ return row
+ raise LookupError(sha)
+
+ def pivot(self):
+ """
+ Return the same object as ChangePointsByMetric.
+ """
+ by_metric = ChangePointsByMetric()
+
+ for row in sorted(self._change_points, key=lambda cpg: cpg.time):
+ assert isinstance(row, ChangePointGroup)
+ # append() does the necessary shuffling into separate columns
+ by_metric.append(row)
+ return by_metric
+
+
+class ChangePointsByTime(ChangePoints):
+ @classmethod
+ def from_dict(cls, cps: dict):
+ """
+ A ChangePointsByTime is stored as a flat list of rows, not keyed by
+ metric, so a dict has no meaningful representation here.
+ """
+ raise TypeError(
+ "ChangePointsByTime doesn't accept a dict. Did you want
ChangePointsByMetric.from_dict()?"
+ )
+
+
+class ChangePointsByMetric(ChangePoints):
+ """
+ Provides same interface as ChangePointsByTime, but internally stores with
metric first.
+ """
+
+ def __init__(self):
+ """
+ The constructor only creates an empty container. To build one from
+ existing data, use the explicit from_list() / from_dict() classmethods.
+ from_list() is inherited from ChangePoints; append() shuffles each
+ group's metrics into their own column.
+ """
+ self._change_points = {}
+
+ # from_dict() is inherited from ChangePoints: it already builds and
returns a
+ # ChangePointsByMetric, which is exactly what we want here.
+
+ def copy(self):
+ """
+ Copy constructor.
+
+ Returns a deep copy of this object.
+ """
+ new_obj = ChangePointsByMetric()
+ new_obj._change_points = {
+ metric: [cpg.copy() for cpg in cpglist]
+ for metric, cpglist in self._change_points.items()
+ }
+ return new_obj
+
+ def pivot(self):
+ """
+ Pivot (metric,time) to (time,metric) so that we return
ChangePointsByTime() objects
+ """
+ intermediate = []
+ for metric, points in self._change_points.items():
+ for cpg in sorted(points, key=lambda cpg: cpg.time):
+ assert isinstance(cpg, ChangePointGroup)
+ intermediate.append(cpg)
+ cp_by_time = ChangePointsByTime()
+ for cpg in sorted(intermediate, key=lambda cpg: cpg.time):
+ cp_by_time.append(cpg)
+ return cp_by_time
+
+ def append(self, cpg: ChangePointGroup):
+ if not isinstance(cpg, ChangePointGroup):
+ raise TypeError("ChangePoints.append() takes as argument one
ChangePointGroup.")
+ for metric in cpg.metrics():
+ if metric not in self._change_points:
+ self._change_points[metric] = []
+ for metric1, cp in cpg.changes.items():
+ if metric1 != cp.metric:
+ raise ValueError(f"metric field is not internally
consistent. {metric1} != {cp.metric} at {cpg.time}")
+ if (not self._change_points[metric]) or cpg.time >
self._change_points[metric][-1].time:
+ self._change_points[metric].append(cpg.select_metrics(metric))
+ else:
+ raise ValueError(
+ "ChangePoints.extend() can only be used such that time is
monotonically increasing"
+ )
+
+ def extend(self, cps: list[ChangePointGroup]):
+ errmsg = "ChangePoints.extend() takes as argument a list of
ChangePointGroup objects."
+ if not isinstance(cps, list):
+ raise TypeError(errmsg)
+ for cpg in cps:
+ if not isinstance(cpg, ChangePointGroup):
+ raise TypeError(errmsg)
+ for metric1, cp in cpg.changes.items():
+ if metric1 != cp.metric:
+ raise ValueError(f"metric field is not internally
consistent. {metric1} != {cp.metric} at {cpg.time}")
+ if metric1 not in self._change_points:
+ self._change_points[metric1] = []
+ if (not self._change_points[metric1]) or cpg.time >
self._change_points[metric1][-1].time:
+ self._change_points[metric1].append(cpg)
+ else:
+ raise ValueError(
+ "ChangePoints.extend() can only be used such that time
is monotonically increasing"
+ )
+
+ def by_time(self):
+ return self.pivot()
+
+ def by_metric(self):
+ return self
+
+ def items(self):
+ return self._change_points.items()
+
+ def keys(self):
+ return self._change_points.keys()
+
+ def __iter__(self):
+ return self.pivot().__iter__()
+
+ def __len__(self):
+ return max([len(cpg) for metric, cpg in self._change_points.items()])
+
+ def __getitem__(self, n):
+ """
+ Returns the same ChangePointGroup as (ChangePointsByTime)[n] would.
+
+ Hence this calls pivot() and is slow.
+ Alternatively please use .at_timestamp(), .at_commit() or
.get_change_points_for_metric() instead.
+ """
+ return self.by_time()[n]
Review Comment:
Yes, I've been thinking it would actually be straightforward to essentially
change ChangePoints class so that it **always** creates and maintains an up to
date copy of both ChangePointsByMetric and ChangePointsByTime. But I intend to
leave that as a follow-up task too and here I will focus on just getting the
API right. Note that what I'm doing here should not be worse than we already
have, as AnalyzedSeries maintans two separate collections of this and it is
possible (although unlikekly) that you create an object where one is computed
(by_metrict) but the other one is not (by_time).
--
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]
