Merge pull request #14 from tpgillam/tg/drop_trailing

Refactor general case into `push!` and `popfirst!`

Author: tpgillam

.JuliaFormatter.toml

@@ -0,0 +1 @@
+style = "blue"

Project.toml

@@ -1,7 +1,7 @@
 name = "AssociativeWindowAggregation"
 uuid = "444271a7-5434-4a02-b82b-0e30a9223c60"
-authors = ["Thomas Gillam <tpgillam@googlemail.com>"]
-version = "0.3.2"
+authors = ["Tom Gillam <tpgillam@googlemail.com>"]
+version = "0.4.0"
 
 [deps]
 DataStructures = "864edb3b-99cc-5e75-8d2d-829cb0a9cfe8"

docs/src/reference/base.md

@@ -1,10 +1,11 @@
 # General window
 
-A state to represent a window of aribtrarily variable capacity.
+A state to represent a window of arbitrarily variable capacity.
 
-Every time a new value is pushed onto the end of the window, we must specify how many values are removed from the front of the window.
+We push new values onto the window with [`Base.push!`](@ref).
+We can remove old values from the window with [`Base.popfirst!`](@ref).
 
 ```@autodocs
-Modules = [AssociativeWindowAggregation]
+Modules = [AssociativeWindowAggregation, Base]
 Pages = ["base.jl"]
 ```

src/base.jl

@@ -72,47 +72,54 @@ end
 WindowedAssociativeOp{T,Op}() where {T,Op} = WindowedAssociativeOp{T,Op,Op}()
 
 """
-    update_state!(
-        state::WindowedAssociativeOp,
-        value,
-        num_dropped_from_window::Integer
-    ) -> state
+    push!(state::WindowedAssociativeOp{T}, value) -> state
 
-Add the specified value to the state, drop some number of elements from the start of the
-window, and return `state` (which will have been mutated).
+Push `value` onto the end of `state`.
 
 # Arguments
 - `state::WindowedAssociativeOp`: The state to update (will be mutated).
 - `value`: The value to add to the end of the window - must be convertible to a `T`.
-- `num_dropped_from_window::Integer`: The number of elements to remove from the front of
-    the window.
+"""
+function Base.push!(state::WindowedAssociativeOp{T,Op,Op!}, value) where {T,Op,Op!}
+    # We want to make sure that we never actually mutate any value object that we pass to
+    # `push!`.
+    # Note that when initialising `state.sum`, we should take a copy if our mutating `Op!`
+    # is different to `Op`. This is because we know that `Op` is necessarily non-mutating.
+    _copy(x) = (Op === Op!) ? x : deepcopy(x)
+
+    # Include the new value in sum and values.
+    state.sum = isempty(state.values) ? _copy(value) : Op!(state.sum, value)
+    push!(state.values, value)
+    return state
+end
 
-# Returns
-- The instance `state` that was passed in.
 """
-Base.@propagate_inbounds function update_state!(
-    state::WindowedAssociativeOp{T,Op,Op!}, value, num_dropped_from_window::Integer
-) where {T,Op,Op!}
-    # Our index into previous_cumsum is advanced by the number of values we drop from the
-    # window.
-    state.ri_previous_cumsum += num_dropped_from_window
+    popfirst!(state::WindowedAssociativeOp, n::Integer=1) -> state
+
+Drop `n` values from the start of `state`.
+"""
+Base.@propagate_inbounds function Base.popfirst!(
+    state::WindowedAssociativeOp{T,Op}, n::Integer=1
+) where {T,Op}
+    n == 0 && return state
+    n >= 0 || throw(ArgumentError("n must be non-negative, got n=$n"))
+
+    # Our index into previous_cumsum is advanced by the number of values we drop.
+    # Note that we do not assign to `ri_previous_cumsum` immediately, since this would make
+    # `state` invalid were we to throw an exception before returning.
+    new_ri_previous_cumsum = state.ri_previous_cumsum + n
 
     # If this has taken us out-of-range of the _previous_cumsum values, we must recompute
     # them.
-    elements_remaining = length(state.previous_cumsum) - state.ri_previous_cumsum
+    elements_remaining = length(state.previous_cumsum) - new_ri_previous_cumsum
 
     if elements_remaining < 0
         # We have overshot the end of previous_cumsum.
 
-        # We may also need to discard elements from values. This could happen if
-        # num_dropped_from_window > 1.
+        # We may also need to discard elements from values. This could happen if n > 1.
         num_values_to_remove = -elements_remaining
         @boundscheck if num_values_to_remove > length(state.values)
-            throw(
-                ArgumentError(
-                    "num_dropped_from_window = $num_dropped_from_window is out of range"
-                ),
-            )
+            throw(ArgumentError("n = $n is out of range"))
         end
 
         # We now generate the partial sum, and set our index back to zero. values is also
@@ -152,18 +159,12 @@ Base.@propagate_inbounds function update_state!(
 
         state.ri_previous_cumsum = 0
         empty!(state.values)
-        # state.sum is now garbage, but we are not going to use it before we recompute it.
+        # WARNING! `state.sum` is now garbage.
+        #   This means that we have to be careful to not use it if `state.values` is empty.
+    else
+        state.ri_previous_cumsum = new_ri_previous_cumsum
     end
 
-    # We want to make sure that we never actually mutate any value object that we pass to
-    # `update_state!`.
-    # Note that when initialising `state.sum`, we should take a copy if our mutating `Op!`
-    # is different to `Op`. This is because we know that `Op` is necessarily non-mutating.
-    _copy(x) = (Op == Op!) ? x : deepcopy(x)
-
-    # Include the new value in sum and values.
-    state.sum = isempty(state.values) ? _copy(value) : Op!(state.sum, value)
-    push!(state.values, value)
     return state
 end
 
@@ -188,6 +189,10 @@ function window_value(state::WindowedAssociativeOp{T,Op})::T where {T,Op}
         # We aren't using the A buffer, either because values is full or the A buffer has
         # not yet been populated.
         state.sum
+    elseif isempty(state.values)
+        # We are using the A buffer, but not the B buffer.
+        # `state.sum` has an uninitialised value.
+        @inbounds(state.previous_cumsum[index])
     else
         Op(@inbounds(state.previous_cumsum[index]), state.sum)
     end

src/fixed.jl

@@ -16,9 +16,7 @@ mutable struct FixedWindowAssociativeOp{T,Op,Op!}
     window_state::WindowedAssociativeOp{T,Op,Op!,CircularBuffer{T}}
     remaining_window::Int
 
-    function FixedWindowAssociativeOp{T,Op,Op!}(
-        window::Integer
-    ) where {T,Op,Op!}
+    function FixedWindowAssociativeOp{T,Op,Op!}(window::Integer) where {T,Op,Op!}
         window < 1 && throw(ArgumentError("Got window $window, but it must be positive."))
         window_state = WindowedAssociativeOp{T,Op,Op!,CircularBuffer{T}}(
             CircularBuffer{T}(window - 1), CircularBuffer{T}(window)
@@ -50,7 +48,8 @@ function update_state!(state::FixedWindowAssociativeOp, value)
 
     # With @inbounds, we assert that num_dropped_from_window will never exceed the size of
     # the window.
-    @inbounds update_state!(state.window_state, value, num_dropped_from_window)
+    @inbounds popfirst!(state.window_state, num_dropped_from_window)
+    push!(state.window_state, value)
     return state
 end
 

src/time.jl

@@ -84,7 +84,8 @@ function update_state!(state::TimeWindowAssociativeOp, time, value)
         num_dropped_from_window += 1
     end
 
-    update_state!(state.window_state, value, num_dropped_from_window)
+    @inbounds popfirst!(state.window_state, num_dropped_from_window)
+    push!(state.window_state, value)
 
     if !state.window_full && num_dropped_from_window > 0
         # The window has now filled; record this for use in `window_full` below.

test/base.jl

@@ -9,4 +9,32 @@
             end
         end
     end
+
+    @testset "simple" begin
+        state = WindowedAssociativeOp{Int64,+}()
+        @test window_size(state) == 0
+
+        push!(state, 1)
+        @test window_size(state) == 1
+        @test window_value(state) == 1
+
+        push!(state, 2)
+        push!(state, 3, 4)
+        @test window_size(state) == 4
+        @test window_value(state) == 10
+
+        popfirst!(state)
+        @test window_size(state) == 3
+        @test window_value(state) == 9
+
+        popfirst!(state, 2)
+        @test window_size(state) == 1
+        @test window_value(state) == 4
+        @test_throws ArgumentError popfirst!(state, 2)
+        @test window_size(state) == 1
+        @test window_value(state) == 4
+
+        popfirst!(state, 1)
+        @test window_size(state) == 0
+    end
 end

test/fixed.jl

@@ -1,6 +1,4 @@
-function test_fixed_window(
-    values, op; op_bang=nothing, approximate_equality::Bool=false
-)
+function test_fixed_window(values, op; op_bang=nothing, approximate_equality::Bool=false)
     # For later comparison, to ensure that we don't mutate any of these
     values_copy = deepcopy(values)