add `consecutive_overlapping_subspans` #19

ericphanson · 2023-09-20T15:00:38Z

And add keep_last=false possibility to consecutive_subspans, to have a way to get consistent behavior between the two.

We could also try to add keep_last=true to consecutive_overlapping_subspans but it is much more confusing what that means when you have overlap.

ericphanson · 2023-09-20T15:01:20Z

src/utilities.jl

    index_groups = Iterators.partition((span.first_index):(span.last_index), n)
+    if !keep_last
+        r = rem(n_samples(span), n)
+        if r != 0


I know the tests test this branch, bc originally I accidentally had r==0 and they failed!

ericphanson · 2023-09-20T15:20:11Z

src/utilities.jl

+function consecutive_overlapping_subspans(span::AlignedSpan, duration::Period,
+                                          hop_duration::Period)
+    n = n_samples(span.sample_rate, duration)
+    m = n_samples(span.sample_rate, hop_duration)
+    return consecutive_overlapping_subspans(span::AlignedSpan, n, m)
+end
+
+function consecutive_overlapping_subspans(span::AlignedSpan, n::Int, m::Int)
+    index_groups = Iterators.partition((span.first_index):(span.last_index - n + 1),
+                                       m)
+    return (AlignedSpan(span.sample_rate, first(I), first(I) + n - 1)
+            for I in index_groups)
+end


this code is untouched from the private package in which it was copied from

ericphanson · 2023-09-20T15:20:25Z

test/utilities.jl

@@ -37,3 +57,47 @@ end

    @test_throws ArgumentError consecutive_subspans(aligned, Millisecond(1))
 end
+
+@testset "consecutive_overlapping_subspans" begin


this whole testset is untouched from the private package in which it was copied from

(new stuff added in 8eabc1e)

ericphanson · 2023-09-20T15:20:47Z

src/utilities.jl

+    consecutive_overlapping_subspans(span::AlignedSpan, duration::Period,
+                                     hop_duration::Period)
+    consecutive_overlapping_subspans(span::AlignedSpan, n::Int, m::Int)
+
+Create an iterator of `AlignedSpan` such that each `AlignedSpan` has
+`n` (calculated as `n_samples(span.sample_rate, duration)` if `duration::Period` is supplied) samples, shifted by
+`m` (calculated as `n_samples(span.sample_rate, hop_duration)` if `hop_duration::Period` is supplied) samples between
+consecutive spans.
+
+!!! warning
+    When `n_samples(span)` is not an integer multiple of `n`, only AlignedSpans with `n`
+    samples will be returned. This is analgous to `consecutive_subspans` with `keep_last=false`, which is not the default behavior for `consecutive_subspans`.
+
+Note: If `hop_duration` cannot be represented as an integer number of samples,
+rounding will occur to ensure that all output AlignedSpans will have the
+same number of samples. When rounding occurs, the output hop_duration will be:
+`Nanosecond(n_samples(samp_rate, hop_duration) / samp_rate * 1e9)`


this docstring is lightly edited from the private package it was taken from

test/utilities.jl

Co-authored-by: Hannah Robertson <[email protected]>

test/utilities.jl

hannahilea

LGTM, contingent on renaming m variable.

Consider adding keep_last to consecutive_overlapping_subspans, or at least filing it as a feature request?

test/utilities.jl

src/utilities.jl

hannahilea · 2023-09-20T15:51:44Z

src/utilities.jl

+!!! warning
+    When `n_samples(span)` is not an integer multiple of `n`, only AlignedSpans with `n`
+    samples will be returned. This is analgous to `consecutive_subspans` with `keep_last=false`, which is not the default behavior for `consecutive_subspans`.


huh. i wonder if we should add keep_last as a kwarg, for parity??

Yeah, I thought about that (see OP). I think it is a bit hard to know exactly what those semantics should be when there's overlap. I guess something like: if there's 8 fold overlap, the last 7 will all be shorter than usual, and the last one will be 1 sample long. But it's a bit weird. I'd rather leave that until someone needs it.

src/utilities.jl

ericphanson · 2023-09-20T16:01:32Z

Filed #20

src/utilities.jl

ararslan · 2023-09-20T16:36:42Z

src/utilities.jl

+    index_groups = Iterators.partition((span.first_index):(span.last_index),
+                                       n_window_samples)


You can use the existing utility function here for clarity/brevity:

Suggested change

index_groups = Iterators.partition((span.first_index):(span.last_index),

n_window_samples)

index_groups = Iterators.partition(indices(span), n_window_samples)

ararslan · 2023-09-20T16:45:09Z

src/utilities.jl

+    index_groups = Iterators.partition((span.first_index):(span.last_index),
+                                       n_window_samples)
+    if !keep_last
+        r = rem(n_samples(span), n_window_samples)


Suggested change

r = rem(n_samples(span), n_window_samples)

n, r = fldmod(n_samples(span), n_window_samples)

Note that currently you're using rem with fld, which have different rounding semantics. You probably want them to match, so to get fld behavior you can use fldmod or equivalently divrem with RoundDown.

does this matter if all quantities are >0?

ararslan · 2023-09-20T16:46:42Z

src/utilities.jl

+            grps = Iterators.take(index_groups, fld(n_samples(span), n_window_samples))
+            return (AlignedSpan(span.sample_rate, first(I), last(I)) for I in grps)


Suggested change

grps = Iterators.take(index_groups, fld(n_samples(span), n_window_samples))

return (AlignedSpan(span.sample_rate, first(I), last(I)) for I in grps)

index_groups = Iterators.take(index_groups, n)

(Uses the definition of n in the previous suggestion.) If you redefine the binding then you don't have to rewrite the comprehension which is otherwise the same as the one outside of this block.

ararslan · 2023-09-20T16:47:35Z

src/utilities.jl

+                                          hop_duration::Period)
+    n_window_samples = n_samples(span.sample_rate, duration)
+    n_hop_samples = n_samples(span.sample_rate, hop_duration)
+    return consecutive_overlapping_subspans(span::AlignedSpan, n_window_samples,


Suggested change

return consecutive_overlapping_subspans(span::AlignedSpan, n_window_samples,

return consecutive_overlapping_subspans(span, n_window_samples,

Was the type assertion there doing something?

copy-paste error

ericphanson requested review from mich11 and hannahilea September 20, 2023 15:00

ericphanson commented Sep 20, 2023

View reviewed changes

ericphanson changed the title ~~add consecutive_overlapping_subspans from SuppressionDetection.jl~~ add consecutive_overlapping_subspans Sep 20, 2023

add function

5d3dd03

ericphanson force-pushed the eph/consecutive_overlapping_subspans branch from fc0fb36 to 5d3dd03 Compare September 20, 2023 15:05

ericphanson commented Sep 20, 2023

View reviewed changes

add tests for Period method

8eabc1e

hannahilea reviewed Sep 20, 2023

View reviewed changes

test/utilities.jl Outdated Show resolved Hide resolved

github-actions bot reviewed Sep 20, 2023

View reviewed changes

test/utilities.jl Outdated Show resolved Hide resolved

test/utilities.jl Outdated Show resolved Hide resolved

test/utilities.jl Outdated Show resolved Hide resolved

test/utilities.jl Outdated Show resolved Hide resolved

ericphanson and others added 2 commits September 20, 2023 17:28

Update test/utilities.jl

fb6a42c

Co-authored-by: Hannah Robertson <[email protected]>

format

4b7b773

github-actions bot reviewed Sep 20, 2023

View reviewed changes

test/utilities.jl Outdated Show resolved Hide resolved

test/utilities.jl Outdated Show resolved Hide resolved

test/utilities.jl Outdated Show resolved Hide resolved

test/utilities.jl Outdated Show resolved Hide resolved

add to docs

ad97829

hannahilea approved these changes Sep 20, 2023

View reviewed changes

ericphanson added 4 commits September 20, 2023 17:55

tweak

266201f

rename m to n_hop_samples

db1e915

rename n to n_window_samples

cf7de0b

format

ee8ee4b

github-actions bot reviewed Sep 20, 2023

View reviewed changes

src/utilities.jl Outdated Show resolved Hide resolved

src/utilities.jl Outdated Show resolved Hide resolved

src/utilities.jl Outdated Show resolved Hide resolved

ericphanson mentioned this pull request Sep 20, 2023

Add keep_last to consecutive_overlapping_subspans? #20

Open

ericphanson commented Sep 20, 2023

View reviewed changes

src/utilities.jl Outdated Show resolved Hide resolved

Update src/utilities.jl

d63309e

ericphanson merged commit 5f9386d into main Sep 20, 2023
8 checks passed

ericphanson deleted the eph/consecutive_overlapping_subspans branch September 20, 2023 16:39

ararslan reviewed Sep 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `consecutive_overlapping_subspans` #19

add `consecutive_overlapping_subspans` #19

ericphanson commented Sep 20, 2023

ericphanson Sep 20, 2023

ericphanson Sep 20, 2023

ericphanson Sep 20, 2023

ericphanson Sep 20, 2023

ericphanson Sep 20, 2023

hannahilea left a comment

hannahilea Sep 20, 2023

ericphanson Sep 20, 2023

ericphanson commented Sep 20, 2023

ararslan Sep 20, 2023

ararslan Sep 20, 2023

ericphanson Sep 20, 2023

ararslan Sep 20, 2023

ararslan Sep 20, 2023

ericphanson Sep 20, 2023

		index_groups = Iterators.partition((span.first_index):(span.last_index),
		n_window_samples)

	index_groups = Iterators.partition((span.first_index):(span.last_index),
	n_window_samples)
	index_groups = Iterators.partition(indices(span), n_window_samples)

	r = rem(n_samples(span), n_window_samples)
	n, r = fldmod(n_samples(span), n_window_samples)

		grps = Iterators.take(index_groups, fld(n_samples(span), n_window_samples))
		return (AlignedSpan(span.sample_rate, first(I), last(I)) for I in grps)

	grps = Iterators.take(index_groups, fld(n_samples(span), n_window_samples))
	return (AlignedSpan(span.sample_rate, first(I), last(I)) for I in grps)
	index_groups = Iterators.take(index_groups, n)

	return consecutive_overlapping_subspans(span::AlignedSpan, n_window_samples,
	return consecutive_overlapping_subspans(span, n_window_samples,

add consecutive_overlapping_subspans #19

add consecutive_overlapping_subspans #19

Conversation

ericphanson commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannahilea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericphanson commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

add `consecutive_overlapping_subspans` #19

add `consecutive_overlapping_subspans` #19