Add ChangePoints parity tests - #173
Conversation
There was a problem hiding this comment.
Pull request overview
Adds parity coverage and fixes behavior inconsistencies between time- and metric-oriented change-point views.
Changes:
- Adds deterministic and Hypothesis-generated parity tests.
- Fixes metric selection, pivot mutation, and metric projection behavior.
- Documents shared conversion views and adds Hypothesis dependencies.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
otava/change_point_divisive/base.py |
Fixes view parity and documents copy semantics. |
tests/change_point_classes_test.py |
Adds parity and mutation tests. |
pyproject.toml |
Adds Hypothesis development dependency. |
uv.lock |
Locks Hypothesis and transitive dependencies. |
Suppressed comments (1)
otava/change_point_divisive/base.py:708
- This validation only checks the outer container. A list containing a non-string metric bypasses the new
TypeErrorpath and later raisesKeyError(or an incidental unhashable-type error), so the two views still do not expose a consistent invalid-input contract. Validate list elements here as well.
if not isinstance(m, list):
if not isinstance(m, str):
raise TypeError(
"ChangePoints.select_metrics() takes as argument a str or a list of str."
)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Add a hand-authored sparse reference case and Hypothesis-generated sparse histories for ChangePointsByTime and ChangePointsByMetric. Read parity covers iteration and indexing, metric enumeration and selection, per-metric reads, timestamp and commit lookup, and pivot/by_time/by_metric conversions. Mutation coverage exercises copy isolation, shared conversion views, append/extend construction, and invalid or out-of-order boundaries. The tests exposed and this commit fixes four parity bugs: - ChangePoints.select_metrics() retained empty timestamp rows and disagreed with the metric view for missing metrics. It now raises consistent KeyError exceptions and omits empty groups. - Both select_metrics() implementations accepted lists without validating every element. Invalid keys could leak into lookup and raise KeyError, including before a later invalid value in a mixed list was examined. Both views now validate all elements first and consistently raise TypeError at the API boundary. - ChangePointsByMetric.pivot() structurally contaminated a source metric bucket when merging metrics at the same timestamp. It now builds shallow group views, so conversion itself is non-mutating while shared ChangePoint semantics remain intact. - ChangePointsByMetric.extend() stored a complete multi-metric group in every metric bucket, causing duplicate metrics during later pivots. It now stores the matching per-metric projection, consistent with append(). The conversion contract is documented as producing shared views, with copy().pivot() providing isolation. Hypothesis is added to the development dependencies for the generated parity coverage.
fda8d15 to
d9fbcb1
Compare
|
@henrikingo I am going to merge this and #171 to unblock #169 and further work. Feel free to take a look async / I will be happy to address any feedback in follow-up PRs. |
|
Thanks, I'm catching up, happy to see so much PR activity. I'll give an update on the mailing list soon. But for now I try to catch up with reviewing and responding to PR and issues. |
Add a hand-authored sparse reference case and Hypothesis-generated sparse histories for
ChangePointsByTimeandChangePointsByMetric.Read parity covers iteration and indexing, metric enumeration and selection, per-metric reads, timestamp and commit lookup, and pivot/by_time/by_metric conversions. Mutation coverage exercises copy isolation, shared conversion views, append/extend construction, and invalid or out-of-order boundaries.
The tests exposed and this commit fixes three parity bugs:
ChangePoints.select_metrics()retained empty timestamp rows and disagreed with the metric view for missing or invalid metrics. It now validates inputs, raises consistentTypeError/KeyErrorexceptions, and omits empty groups.ChangePointsByMetric.pivot()structurally contaminated a source metric bucket when merging metrics at the same timestamp. It now builds shallow group views, so conversion itself is non-mutating while shared ChangePoint semantics remain intact.ChangePointsByMetric.extend()stored a complete multi-metric group in every metric bucket, causing duplicate metrics during later pivots. It now stores the matching per-metric projection, consistent with append().In addition, this PR documents that conversions are shared views and that
copy().pivot()provides isolation. This PR also adds Hypothesis to the development dependencies.