Add intersect_spans function

[chetchavat]

This was defined in an internal repo and was deemed useful, so we're putting it in!

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1214948858085077

[ararslan]

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]

if it helps any, the original function was called innerspan

[kleinschmidt]

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]

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]

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.

[ararslan]

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]

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

[kleinschmidt]

sorry to throw another contender into the ring

[kleinschmidt]

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]

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

[kleinschmidt]

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