henrikingo commented on code in PR #161:
URL: https://github.com/apache/otava/pull/161#discussion_r3499999698
##########
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:
Btw, thought about this after sleeping on it...
I think these to_json/from_json methods are currently overloaded. For
example the rounded=True functionality is added (by me) because someone (me)
used these methods to produce json that is sent to an API and a user interface.
But I realized when working on this patch, that should rather be done in the
Report class (which I don't), and these methods should be limited to
serialization and deserialization, such as persisting the data to a database.
If we could separate and limit functionality like that, perhaps these functions
could be simplified a lot and there wouldn't be any issue.
--
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]
