Skip to content

Add ChangePoints parity tests - #173

Merged
Gerrrr merged 1 commit into
masterfrom
changepoints-parity-tests
Aug 24, 2026
Merged

Add ChangePoints parity tests#173
Gerrrr merged 1 commit into
masterfrom
changepoints-parity-tests

Conversation

@Gerrrr

@Gerrrr Gerrrr commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

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 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 consistent TypeError/KeyError exceptions, 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 TypeError path and later raises KeyError (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.

Comment thread otava/change_point_divisive/base.py
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.
@Gerrrr
Gerrrr force-pushed the changepoints-parity-tests branch from fda8d15 to d9fbcb1 Compare August 20, 2026 01:35
@Gerrrr
Gerrrr marked this pull request as ready for review August 20, 2026 01:39
@Gerrrr
Gerrrr requested a review from henrikingo August 20, 2026 01:39
@Gerrrr

Gerrrr commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

@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.

@Gerrrr
Gerrrr merged commit 5f6f93c into master Aug 24, 2026
6 checks passed
@henrikingo

Copy link
Copy Markdown
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants