fix: handle zero-duration clusters in diff_sessions (closes #79)#87
Closed
conorluddy wants to merge 1 commit into
Closed
fix: handle zero-duration clusters in diff_sessions (closes #79)#87conorluddy wants to merge 1 commit into
conorluddy wants to merge 1 commit into
Conversation
Closes #79. The divide-by-zero guard in diff_sessions() used a bare `continue`, silently dropping clusters whose A-side max_duration_ms was 0. A real regression where a previously-silent cluster started hanging (0 → N ms) was lost from the diff entirely. Replace with explicit 4-way branching: - both zero → stable - A=0, B>0 → drift, delta_pct = +inf (treated as worsened) - A>0, B=0 → drift, delta_pct = -100.0 (fully improved) - both nonzero → existing percentage logic, unchanged format_diff() renders the inf delta as `new` instead of `+inf%` for readability. Tests: 5 new in tests/test_diff.py covering each branch + a regression guard that the standard nonzero path still uses the drift threshold, plus format_diff render assertion for the inf case. Verification: ruff/black clean; 93 passed (88 → 93).
Owner
Author
|
Superseded by #90 — same commits combined into one PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #79.
Problem
diff_sessions()had a divide-by-zero guard that did a barecontinue:A zero-duration cluster in session A simply vanished from the diff. Worse, a real regression — a previously-silent cluster (0 ms) that started hanging (e.g. 800 ms in B) — was dropped entirely instead of being flagged as drift.
Fix
Replace with explicit 4-way branching:
A == 0, B == 0stable++A == 0, B > 0delta_pct = float("inf")(worsened, "new signal")A > 0, B == 0delta_pct = -100.0(fully improved)A > 0, B > 0The
infdelta is special-cased informat_diffto render asnewrather than+inf%.Tests
5 new in
tests/test_diff.py:test_diff_zero_to_zero_counts_as_stabletest_diff_zero_to_nonzero_is_drift_with_inf_deltatest_diff_nonzero_to_zero_is_drift_with_minus_one_hundredtest_diff_nonzero_unchanged_still_uses_threshold(regression guard)test_format_diff_renders_inf_delta_as_newA small helper
_zero_dur_summary()buildsSessionSummaryinstances with controlledmax_duration_msper cluster — keeps the test isolated from the event-aggregation pipeline.Verification