Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses incorrect behavior in Interval.union when interval lists contain overlapping intervals (fixing #1519) and introduces stricter validation intended to ensure interval lists are well-formed.
Changes:
- Fix
Interval.unionstart/stop detection logic for overlapping intervals and add internal consistency checks. - Add
_extract_only+_validate_intervalsto validate extracted interval-like inputs. - Update interval helper tests to cover the overlapping-union case and adjust existing expected values.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/spyglass/common/common_interval.py |
Updates union algorithm and introduces interval extraction + validation helpers. |
tests/common/test_interval_helpers.py |
Updates/extends test cases for Interval.union behavior. |
Comments suppressed due to low confidence (1)
tests/common/test_interval_helpers.py:379
- New interval validation behavior is introduced via
_validate_intervals, but there are no tests asserting that invalid intervals (e.g.[start > stop], including the single-interval case) raise aValueError, or that unsorted-but-valid inputs are handled as intended. Adding a focused test (or two) here would lock in the new contract and prevent regressions.
@pytest.mark.parametrize(
"one, two, expected_result",
[
(
np.array([[0, 3]]),
np.array([[2, 4]]),
np.array([[0, 4]]),
),
(
np.array([[0, 1]]),
np.array([[2, 4]]),
np.array([[0, 1], [2, 4]]),
),
(
np.array([[1, 3], [9, 10]]),
np.array([[0, 2]]),
np.array([[0, 3], [9, 10]]),
),
(
np.array([[0, 1]]),
np.array([[2, 1e11]]),
np.array([[0, 1], [2, 1e11]]),
),
],
)
def test_interval_list_union(interval_obj, one, two, expected_result):
ret = interval_obj(one).union(two).times
assert np.array_equal(ret, expected_result), "Problem with Interval.union"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Description
Interval.unionwhen overlapping intervals existIntervals[start, stop]value withstart <= stopChecklist:
CITATION.cffaltersnippet for release notes.CHANGELOG.mdwith PR number and description.