Confidence indicators - #450
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughConfidence indicators are computed per changepoint using Welch’s t-test and Cohen’s d, stored in analysis results, serialized through output and reporting paths, documented, and covered by new statistical and formatter tests. ChangesConfidence Indicators
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
c72c140 to
a45c679
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
orion/utils.py (1)
843-856: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInconsistent
percentage_changeformatting between rows with and without confidence data.
base_record["percentage_change"]is left as a raw float unless a confidence label exists, in which case it becomes a formatted string (f"{pct:.2f}% ({conf['label']})"). This means the JUnit tabular output shows unformatted numeric values for some rows and a nicely formatted"%" + labelstring for others in the same column, which looks inconsistent to a reader of the generated table.💚 Proposed fix: format percentage_change consistently regardless of confidence presence
base_record = { uuid_field: record[uuid_field], "timestamp": datetime.fromtimestamp(record["timestamp"], timezone.utc).strftime( "%Y-%m-%dT%H:%M:%SZ" ), metric_name: record["metrics"][metric_name]["value"], "is_changepoint": bool(record["metrics"][metric_name]["percentage_change"]), - "percentage_change": record["metrics"][metric_name]["percentage_change"], + "percentage_change": f"{record['metrics'][metric_name]['percentage_change']:.2f}%", } conf = record["metrics"][metric_name].get("confidence") if conf and conf.get("label"): - pct = base_record["percentage_change"] - base_record["percentage_change"] = f"{pct:.2f}% ({conf['label']})" + base_record["percentage_change"] = ( + f"{record['metrics'][metric_name]['percentage_change']:.2f}% " + f"({conf['label']})" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orion/utils.py` around lines 843 - 856, Update create_record so base_record["percentage_change"] is consistently formatted to two decimal places with a percent sign for every row, then append the confidence label when available. Preserve the existing confidence lookup and label behavior while ensuring rows without confidence data use the same percentage formatting.
🧹 Nitpick comments (4)
orion/run_test.py (1)
252-254: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConfidence is computed unconditionally up front, then re-computed again when window expansion runs.
confidence_by_metricis computed at Lines 252-254 immediately after the initial analysis. Ifhas_early_changepoint_rawtriggers window expansion,confidence_by_metricis unconditionally recomputed again in every sub-branch (Lines 324-327, 337-340, 354-357), making the first computation wasted work whenever expansion happens.Consider deferring the initial
compute_confidencecall until after the expansion decision is finalized (or guard it behind theelsepath that skips expansion).Also applies to: 324-327, 337-340, 354-357
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orion/run_test.py` around lines 252 - 254, Defer the initial compute_confidence call that assigns confidence_by_metric until the window-expansion decision is finalized, or execute it only on the path that skips expansion. Ensure expansion branches reuse their existing recomputation and avoid calculating confidence before has_early_changepoint_raw triggers expansion.orion/tests/test_confidence.py (1)
162-212: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing edge-case coverage for NaN values preceding a changepoint.
None of these
compute_confidencetests exercise a dataframe withNaNgaps in the metric column before the changepoint index — the scenario that surfaces thedropna()/positional-index misalignment flagged inorion/confidence.py. Based on path instructions ("Ensure test coverage for edge cases"), consider adding a regression test for this once the underlying fix lands.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orion/tests/test_confidence.py` around lines 162 - 212, Add a regression test to TestComputeConfidence for a metric column containing NaN values before a changepoint, invoking compute_confidence and asserting the result remains correctly aligned with that changepoint and does not misclassify or crash. Use the existing ConfidenceResult assertions and CMR/EDIVISIVE setup as appropriate, preserving the expected behavior after the confidence.py indexing fix.Source: Path instructions
orion/tests/test_formatters.py (1)
607-607: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
next(...)over single-element list slicing (Ruff RUF015).Static analysis flags
[r for r in parsed if r["is_changepoint"]][0]at Lines 607, 639, and 655;next(r for r in parsed if r["is_changepoint"])avoids building the full filtered list.Also applies to: 639-639, 655-655
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orion/tests/test_formatters.py` at line 607, Replace the single-element list comprehensions indexed with [0] in the changepoint lookups at the affected test cases with next(...) over the filtered generator. Preserve the existing selection of the first record where r["is_changepoint"] is true.Source: Linters/SAST tools
orion/confidence.py (1)
58-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWeight Cohen’s d pooled standard deviation by sample size.
Use
sqrt(((n_before - 1) * std_before**2 + (n_after - 1) * std_after**2) / (n_before + n_after - 2))before dividing by pooledstd. The current unweighted average only matches Cohen’s d whenn_before == n_after; with unequal windows it can shiftcohens_dby roughly 25–40% or more, changing the confidence label.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orion/confidence.py` around lines 58 - 90, Update _compute_stats to calculate pooled_std using the sample-size-weighted formula with n_before and n_after, dividing by n_before + n_after - 2 before computing cohens_d. Preserve the existing zero-pooled-standard-deviation handling and subsequent confidence-label logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/usage.md`:
- Around line 323-331: Update the fenced code block containing the “Affected
Metrics” table to declare the text language, using the existing table content
unchanged.
In `@orion/confidence.py`:
- Around line 7-8: Add SciPy as an explicit pinned dependency in each project
dependency declaration: pyproject.toml, requirements.txt, and setup.py’s
install_requires. Keep the version consistent across all declarations so the
direct scipy.stats import in orion/confidence.py installs deterministically.
- Around line 103-130: Update compute_confidence so changepoint positions remain
aligned with the original dataframe after removing NaN values: preserve the
original row positions or translate each cp.index to its corresponding position
in the filtered data before calling _get_segments. Ensure non-Isolation Forest
changepoints split at the correct dataframe position and retain their associated
confidence results.
---
Outside diff comments:
In `@orion/utils.py`:
- Around line 843-856: Update create_record so base_record["percentage_change"]
is consistently formatted to two decimal places with a percent sign for every
row, then append the confidence label when available. Preserve the existing
confidence lookup and label behavior while ensuring rows without confidence data
use the same percentage formatting.
---
Nitpick comments:
In `@orion/confidence.py`:
- Around line 58-90: Update _compute_stats to calculate pooled_std using the
sample-size-weighted formula with n_before and n_after, dividing by n_before +
n_after - 2 before computing cohens_d. Preserve the existing
zero-pooled-standard-deviation handling and subsequent confidence-label logic.
In `@orion/run_test.py`:
- Around line 252-254: Defer the initial compute_confidence call that assigns
confidence_by_metric until the window-expansion decision is finalized, or
execute it only on the path that skips expansion. Ensure expansion branches
reuse their existing recomputation and avoid calculating confidence before
has_early_changepoint_raw triggers expansion.
In `@orion/tests/test_confidence.py`:
- Around line 162-212: Add a regression test to TestComputeConfidence for a
metric column containing NaN values before a changepoint, invoking
compute_confidence and asserting the result remains correctly aligned with that
changepoint and does not misclassify or crash. Use the existing ConfidenceResult
assertions and CMR/EDIVISIVE setup as appropriate, preserving the expected
behavior after the confidence.py indexing fix.
In `@orion/tests/test_formatters.py`:
- Line 607: Replace the single-element list comprehensions indexed with [0] in
the changepoint lookups at the affected test cases with next(...) over the
filtered generator. Preserve the existing selection of the first record where
r["is_changepoint"] is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fc77f491-75e0-4f5e-895c-b8d0e4e6f3fc
📒 Files selected for processing (12)
docs/usage.mdorion/confidence.pyorion/pipeline/analysis_result.pyorion/pipeline/formatters/base.pyorion/pipeline/formatters/json_formatter.pyorion/reporting/standalone.pyorion/reporting/summary.pyorion/run_test.pyorion/tests/test_confidence.pyorion/tests/test_formatters.pyorion/tests/test_matcher_batch.pyorion/utils.py
|
@coderabbitai please review again |
|
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
orion/tests/test_formatters.py (2)
662-717: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGrowing duplication of inline
ConcreteFormatterstub.
TestRegressionDataConfidenceadds two more copies of the identicalConcreteFormatterstub class (lines 674-686, 700-712), on top of the three already inTestExtractRegressionData(lines 80-91, 109-120, 129-140). Any future change to theBaseFormatterABC surface now requires updating 5 identical stubs across this file. Consider hoisting a single shared stub (module-level class or apytestfixture) once, for reuse across all these tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orion/tests/test_formatters.py` around lines 662 - 717, Hoist the repeated ConcreteFormatter implementation into one shared module-level test stub or pytest fixture, then update the five tests in TestExtractRegressionData and TestRegressionDataConfidence to reuse it instead of defining inline classes. Preserve the existing no-op method behavior and BaseFormatter interface.
593-660: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
next()instead of list-comprehension-then-index (Ruff RUF015).Static analysis flags the
[r for r in parsed if r["is_changepoint"]][0]pattern at lines 607, 639, and 655. Usingnext()avoids materializing the full list and is the idiomatic fix Ruff suggests.🧹 Proposed fix
- cp_record = [r for r in parsed if r["is_changepoint"]][0] + cp_record = next(r for r in parsed if r["is_changepoint"])(apply the same change at lines 639 and 655)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@orion/tests/test_formatters.py` around lines 593 - 660, Replace the list-comprehension-then-index lookups for changepoint records in TestJsonConfidence methods test_changepoint_has_confidence_object, test_no_confidence_data_no_key, and test_insufficient_data_in_json with next() lookups, preserving the existing record selection and assertions.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@orion/tests/test_confidence.py`:
- Around line 174-182: The test_index_aligned_with_changepoints assertion only
checks the result count, not alignment. Update it to assert the ordered
changepoint-to-sample-size pairs are [(3, 7), (5, 5)], preserving the existing
compute_confidence invocation.
---
Nitpick comments:
In `@orion/tests/test_formatters.py`:
- Around line 662-717: Hoist the repeated ConcreteFormatter implementation into
one shared module-level test stub or pytest fixture, then update the five tests
in TestExtractRegressionData and TestRegressionDataConfidence to reuse it
instead of defining inline classes. Preserve the existing no-op method behavior
and BaseFormatter interface.
- Around line 593-660: Replace the list-comprehension-then-index lookups for
changepoint records in TestJsonConfidence methods
test_changepoint_has_confidence_object, test_no_confidence_data_no_key, and
test_insufficient_data_in_json with next() lookups, preserving the existing
record selection and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 79acabe9-4d7a-491c-82c7-51d4376b4780
📒 Files selected for processing (15)
docs/usage.mdorion/confidence.pyorion/pipeline/analysis_result.pyorion/pipeline/formatters/base.pyorion/pipeline/formatters/json_formatter.pyorion/reporting/standalone.pyorion/reporting/summary.pyorion/run_test.pyorion/tests/test_confidence.pyorion/tests/test_formatters.pyorion/tests/test_matcher_batch.pyorion/utils.pypyproject.tomlrequirements.txtsetup.py
🚧 Files skipped from review as they are similar to previous changes (10)
- orion/pipeline/formatters/json_formatter.py
- orion/reporting/standalone.py
- orion/reporting/summary.py
- orion/pipeline/formatters/base.py
- orion/pipeline/analysis_result.py
- docs/usage.md
- orion/utils.py
- orion/tests/test_matcher_batch.py
- orion/run_test.py
- orion/confidence.py
0d75679 to
ea82944
Compare
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
ea82944 to
6d06dc7
Compare
Removes duplicated scenarios from test.bats Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
|
I like this change, can we have a flag to output the change points only after a certain confidence score? That way integration in CI will be simple. |
I think regardless of the confidence score we let CI stay with all found regressions - i think the additional context will help Chaibot make better decisions long term. |
Correct, but more precise we are the better it is. From the token usage reduction standpoint 💸 |
yeah - that could help reduce the token aspect -- we could pass that via orion-mcp |
|
@chentex this looks great! One aspect I think we can change (not here, but on a follow-on) is the term |
|
/lgtm |
sjug
left a comment
sjug
left a comment
There was a problem hiding this comment.
Requesting changes mainly because of two issues in how confidence is calculated: the current approach can make random noise look like a real change, and can label a real change as noise. Details are in the inline comments, along with smaller suggestions on output format, code structure, and tests.
This PR also removes roughly 600 lines from test.bats and adds an unrelated pylint suppression in test_matcher_batch.py. The deleted scenarios remain in test-local.bats, so coverage is preserved, but these cleanups should be acknowledged in the PR description or moved to a separate change.
| std_after=float(np.std(after, ddof=1)) if n_after > 1 else None, | ||
| ) | ||
|
|
||
| _, p_value = stats.ttest_ind(before, after, equal_var=False) |
There was a problem hiding this comment.
The changepoint was selected from these same values, so testing it again with an ordinary Welch test makes random noise look significant too often. The reported ci_95 has the same problem. Please calibrate by resampling the full detector-and-selection pipeline or by using separate confirmation data. Otava's existing window statistics may be reusable descriptively, but only if their selection guarantees are understood.
There was a problem hiding this comment.
Otava's existing window statistics may be reusable
that would be nice! are there suggestions on how we could adjust the existing windowing?
There was a problem hiding this comment.
Yes. For the Otava changepoints, I would reuse the local before/after statistics already attached to ChangePoint.stats, or preserve the exact segment boundaries and sample sizes Otava used before Orion filters changepoints. That fixes the current prefix/suffix problem. We should not derive the boundaries from the filtered changepoint list because a neighboring boundary may already have been removed.
This only fixes the comparison window. Otava's p-value is also involved in selecting the changepoint, so it is not independently calibrated. In the short term, I would report the local means, standard deviations, and effect size as descriptive context without presenting the p-value or interval as calibrated confidence. Fully calibrated inference would require rerunning the complete detector-and-selection process on resampled data that contains no real change, or using held-out runs.
There was a problem hiding this comment.
changing the underlying algo could introduce new regressions in the pipeline, right? where the current implementation will categorize the regressions likelihood.
There was a problem hiding this comment.
No algorithm change needed. The suggestion is to reuse statistics Otava already computes internally when it finds each changepoint, purely for the confidence annotation. Detection results would be identical.
| """Split data into before/after segments based on algorithm type.""" | ||
| if algorithm_name == cnsts.CMR: | ||
| return data[:-1], data[-1:] | ||
| return data[:changepoint_index], data[changepoint_index:] |
There was a problem hiding this comment.
This replaces the detector's local window with the entire prefix and suffix. A later recovery or reversal can therefore cancel a genuine earlier shift and label it "Noise." Please compare segments bounded by neighboring changepoints, or reuse the detector's window statistics.
There was a problem hiding this comment.
The neighboring-window change is directionally right, but cp_indices is built from cps after Orion filters changepoints by direction, threshold, ACK state, and correlation. If a recovery changepoint is filtered out, its boundary disappears and the original dilution problem returns.
I reproduced this with a 0 -> +10 -> -10 series. With only the reported first changepoint, the comparison gives d=0.002 and p=0.992. With both detector boundaries, it gives d=120.65 and p=5.4e-69.
Please preserve the raw detector boundaries or local statistics before Orion filters the changepoints, and use those raw boundaries for the comparison windows.
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
|
Thanks for the follow-up. Most of the original presentation, serialization, and maintainability concerns are addressed. I am keeping changes requested for the two original statistical blockers: post-selection calibration and comparison boundaries lost during changepoint filtering. The CMR raw-data issue also needs correction. Before merge, please also refresh the PR description. It still shows the old "Likely real"/"Noise" labels and old JUnit layout, and it does not mention the test.bats deduplication or unrelated pylint suppression. |
|
@afcollins Requested changes, not comments. As I summarized in the comment prior to yours. |
Updates test scenarios CMR data is now used with the original dataframe is_changepoint is included as a per metric value also Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Vicente Zepeda Mas <vzepedam@redhat.com>
5 participants
Type of change
Description
Every detected changepoint is automatically annotated with a statistical confidence indicator based on:
Labels
Labels are driven by Cohen's d thresholds (Cohen 1988), with p-value shown inline for reference:
Large shift (d=1.20, p=0.001)Moderate shift (d=0.60, p=0.03)Small shift (d=0.30, p=0.02)Negligible shift (d=0.10, p=0.85)Degenerate variance — shift detected but effect size undefinedInsufficient dataAnomaly detection — shift confidence not applicableSegment boundaries
For E-Divisive, before/after segments are bounded by raw detector boundaries (all changepoints found by Otava before Orion applies direction/threshold/ACK/correlation filtering). This prevents a filtered recovery changepoint from dilating the comparison win
dow and masking a genuine earlier shift.
For CMR, confidence is computed against the original (uncollapsed) dataframe, preserving the baseline standard deviation across all historical runs rather than collapsing to a single averaged row.
Per-metric
is_changepointEach metric object in JSON output now includes an explicit
is_changepointboolean field, replacing the previous inference frompercentage_change != 0.Additional changes
ci_95field is always present in JSON output (set tonullwhen confidence cannot be computed)pyproject.toml,requirements.txt,setup.py)test.bats: removed tests duplicated withtest-local.batstest-local.bats: assertions updated to match new label format and percent formattingtest_matcher_batch.py: addedpylint: disable=protected-accesssuppression forTestGetNesteddocs/usage.mdRelated Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Output Improvements
Chores
Output Examples
Text
JSON
JUNIT