Skip to content

Add intersect_spans function #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chetchavat merged 11 commits into from
May 27, 2026
Merged

Add intersect_spans function #72

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
dec62b3
intersect_spans function
chetchavat May 19, 2026
1e6e53d
export function
chetchavat May 19, 2026
b8a37f4
test intersection
chetchavat May 19, 2026
c89d6a0
remove whitespacre
chetchavat May 19, 2026
52715f5
update project
chetchavat May 19, 2026
934eab0
Update src/TimeSpans.jl
chetchavat May 20, 2026
1c72dbd
Update src/TimeSpans.jl
chetchavat May 20, 2026
9c26ca2
Update test/runtests.jl
chetchavat May 21, 2026
69e0134
Update test/runtests.jl
chetchavat May 21, 2026
0bcfd50
change to intersect, avoid collision
chetchavat May 21, 2026
0dd7b07
Update src/TimeSpans.jl
chetchavat May 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name = "TimeSpans"
uuid = "bb34ddd2-327f-4c4a-bfb0-c98fc494ece1"
authors = ["Beacon Biosignals, Inc."]
version = "1.1.0"
version = "1.2.0"

[deps]
ArrowTypes = "31f734f8-188a-4ce0-8406-c8a06bd891cd"
Expand Down
15 changes: 14 additions & 1 deletion src/TimeSpans.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ using Statistics

export TimeSpan, start, stop, istimespan, translate, overlaps,
shortest_timespan_containing, duration, index_from_time,
time_from_index, merge_spans!, merge_spans, invert_spans
time_from_index, merge_spans!, merge_spans, invert_spans,
intersect_spans

const NS_IN_SEC = Dates.value(Nanosecond(Second(1))) # Number of nanoseconds in one second

Expand Down Expand Up @@ -399,6 +400,18 @@ function invert_spans(spans, parent_span)
return gaps
end

"""
intersect_spans(span_1, span_2)

Returns a timespan that consists of the intersection of two timespans.

@ararslan ararslan May 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder whether "intersect" is really the right verb here. Describing it as the intersection of the two spans makes sense, but it kind of suggests to me that other set terminology would be applicable, but I don't believe there are any such cases. For example, TimeSpans.jl uses overlaps(a, b) rather than !isdisjoint(a, b), and TimeSpans.contains(a, b) rather than issubset(b, a). I could imagine that given that the query for "is there a nonempty intersection" is overlaps, "what is the intersection" would be overlap. But if overlap returns a span, it's not immediately obvious whether overlaps then is plural or present tense. I have no answers, only quandaries.

@chetchavat chetchavat May 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if it helps any, the original function was called innerspan

@kleinschmidt kleinschmidt May 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is really the set-theoretic operation though, (other than the error-if-disjoint lol).

my gripe with intersect_spans is the other part, the _spans: the other _spans functions operate on iterables of spans rather than pairs; the pairwise overlaps, contains don't have _spans.

I think TimeSpans.intersect is fine, it has the same semantics as the base function and not exporting it makes the scope clear a la contains

@kleinschmidt kleinschmidt May 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll also add that Base.contains tends to come along with other collection-y methods, but the namespacing makes it pretty clear that it's different

@ararslan ararslan Jul 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very, very late to the party but IMO TimeSpans.contains should be renamed to contains_span. (Actually, in basically all code I've written where I've needed that function, I do using TimeSpans: contains as contains_span, haha.) Having only part of the API namespaced feels inconsistent, and having a package with a function that has the same name as a Base function is really confusing. I strongly prefer intersect_spans over TimeSpans.intersect.

"""
Comment thread
chetchavat marked this conversation as resolved.
function intersect_spans(span_1, span_2)
overlaps(span_1, span_2) || throw(ArgumentError("provided spans must overlap"))

spans = [span_1, span_2]
return TimeSpan(maximum(start, spans), minimum(stop, spans))
end

@ararslan ararslan May 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO it's a bit clearer not to express things as array reductions on a two-element array. Also, the other functions in this package that operate on two span arguments call the arguments a and b, so we might as well follow suit here.

Suggested change
function intersect_spans(span_1, span_2)
overlaps(span_1, span_2) || throw(ArgumentError("provided spans must overlap"))
spans = [span_1, span_2]
return TimeSpan(maximum(start, spans), minimum(stop, spans))
end
function intersect_spans(a, b)
overlaps(a, b) || throw(ArgumentError("provided spans must overlap"))
return TimeSpan(max(start(a), start(b)), min(stop(a), stop(b)))
end

@kleinschmidt kleinschmidt May 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree, max/min are more idiomatic when dealing with two things. it also makes the intent really clear.


#####
##### Package extensions (TODO: remove this section once we require Julia 1.9+)
#####
Expand Down
12 changes: 12 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,18 @@ end
@test test_vec == []
end

@testset "intersect_spans" begin
test_span_1 = TimeSpan(10, 100)
test_span_2 = TimeSpan(20, 80)
test_span_3 = TimeSpan(80, 120)
non_intersecting_span = TimeSpan(101, 150)

@test intersect_spans(test_span_1, test_span_2) == test_span_2
@test intersect_spans(test_span_1, test_span_3) == TimeSpan(80, 100)

@test_throws ArgumentError intersect_spans(test_span_1, non_intersecting_span)
end

@testset "extensions" begin
@testset "ArrowTypes" begin
using ArrowTypes
Expand Down
Loading