Allow using AbstractVectors{T} rather than Array{T, 1} only

[artemsolod]

I updated types of methods to be more permissive (AbstractVector{T} instead of Array{T, 1}). This allows to run functions on views for example. Hope this does not break anything, tests passed without problems.

[Firionus]

Hey @artemsolod :wave:,

first thanks for putting in your PR - it's great to see people are actually using my code and wanting to improve it :blush:.

I like your idea and will merge the changes to master. However, there are 2 issues I'd like to address before releasing it in 0.2:

• What is your use case for this? Could you add a little stripped down test to demo it? (just create a new PR)
• There's currently a heated discussion in the forum about compatibility within the ecosystem, specifically mentioning issues with OffsetArrays (https://discourse.julialang.org/t/discussion-on-why-i-no-longer-recommend-julia-by-yuri-vishnevsky/81151/95). Your change allows the user to input arrays without 1-based linear indexing, which is assumed in this library at the moment, see for example here: https://github.com/Firionus/FastRunningMedian.jl/blob/18c69983883a096b5b1292daf8012d2c0fbd5169/src/FastRunningMedian.jl#L365
  Therefore, I'll create a new issue to add test cases for OffsetArrays (I'm also a bit worried about AbstractSparseVector :thinking: ). Feel free to continue contributing there.

[artemsolod]

That's a good point, maybe adding argument check like Base.require_one_based_indexing(input) would be a reasonable solution? This is suggested in a follow-up discussion https://discourse.julialang.org/t/offsetarrays-inbounds-and-confusion/81295/23. I have not used AbstractSparseVector so I am not quite sure what problem might arise there though.

Update: I see now that you've updated the code to support OffsetArray, so I guess this suggestion is irrelevant now (unless performance have suffered somehow)

[Firionus]

Yeah, I considered require_one_based_indexing, but once I switched to a stateful iterator the code ended up even more neat. Instead of keeping an index around, you just popfirst! whenever you need a value. I added an issue as a reminder to re-run the Benchmarks before the release.

[Firionus]

I just released v0.2.0 which contains this change. Thanks for your contribution :rocket:

I noticed some slight performance regressions (< 20 % on large windows) in my benchmark between my previous run and now. But re-running now at an earlier commit (18c699) shows that while there is probably a performance regression from v0.1.0 to v0.2.0, it is only about 3 % to 7 % due to changes in my code. Not sure where that comes from, since iterators instead of counters shouldn't be less performant and the stateful API parts haven't changed.

If I find some additional time over the coming days I might look into this and also tackle the small-window implementation I've been eyeing for a while. But performance optimization is a deep dark hole that one should only go down carefully :smile_cat: