Fix argmax, findmax, findXwithfirst, and expand testing

[mkitti]

This pull request adds more tests including randomly generated ChainVectors of Int, Float64, and Rational, incorporates #98 .

Functions fixed

• argmin (Already fixed)
• argmax
• findmin (Already fixed)
• findmax
• findXwithfirst

Fixed #101

[Seelengrab]

Ah yes, the inconsistency of argmax(x) vs argmax(foo, x) strikes again... I'll have to get around to rebasing JuliaLang/julia#41339

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.73%. Comparing base (fa840f9) to head (3a6f915).
Report is 4 commits behind head on main.

❗ Current head 3a6f915 differs from pull request most recent head 750cb8f

Please upload reports for the commit 750cb8f to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
- Coverage   94.82%   94.73%   -0.10%     
==========================================
  Files           5        5              
  Lines        1043     1044       +1     
==========================================
  Hits          989      989              
- Misses         54       55       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nhz2]

Instead of a hard-coded set of test vectors, could a few 1000 be generated when the test runs? That might catch a few more edge cases.

Also, I think it would be clearer to only test Int here, to avoid NaN and floating point rounding.

[mkitti]

I think that is part of @Seelengrab 's Supposition.jl is supposed to do. I'm just trying to get this pull request through before I start messing with adding test dependencies, but we seem to be stuck here.

[mkitti]

With == the failing tests in eee286e are

  (x, y) = ([-8.5, -1.2, -3.8, 7.5, 8.2, 7.5, -5.3, -2.7, 0.6, -6.2, 6.1, 1.4], [-8.5, -1.2, -3.8, 7.5, 8.2, 7.5, -5.3, -2.7, 0.6, -6.2, 6.1, 1.4])                                                                                                                                                           |   32     1     33  1.3s
  (x, y) = ([-7.2, 8.1, 2.3, 7.5, -8.4, -5.7], [-7.2, 8.1, 2.3, 7.5, -8.4, -5.7])                                                                                                                                                                                                                             |   32     1     33  0.0s
  (x, y) = ([-3.7, 7.8, -5.0, 0.1, 5.0, -4.1, -1.6, -0.9, 8.7, -7.8], [-3.7, 7.8, -5.0, 0.1, 5.0, -4.1, -1.6, -0.9, 8.7, -7.8])                                                                                                                                                                               |   32     1     33  0.0s
  (x, y) = ([8.6, -2.0, 8.0, 3.4, 3.3, 1.0, 5.4, -2.6, -4.7, 4.4, 4.4, 7.9], [8.6, -2.0, 8.0, 3.4, 3.3, 1.0, 5.4, -2.6, -4.7, 4.4, 4.4, 7.9])                                                                                                                                                                 |   32     1     33  0.0s
  (x, y) = ([7.6, 5.9, 7.9, -8.8, -1.5, -0.4, 6.0, -5.1, -0.4, 4.4, 7.3], [7.6, 5.9, 7.9, -8.8, -1.5, -0.4, 6.0, -5.1, -0.4, 4.4, 7.3])                                                                                                                                                                       |   32     1     33  0.0s
  (x, y) = ([3.2, -3.2, 1.2, -1.2, -2.1, 0.5, 6.2, 2.9, -8.1, 5.8, 4.8, -3.4, -3.1], [3.2, -3.2, 1.2, -1.2, -2.1, 0.5, 6.2, 2.9, -8.1, 5.8, 4.8, -3.4, -3.1])                                                                                                                                                 |   32     1     33  0.0s
  (x, y) = ([-7.5, -1.5, -5.8, 8.4, -8.4, -1.9, 2.3, -0.8, -8.5, 0.2, 0.5, -7.4, 2.1, -3.9], [-7.5, -1.5, -5.8, 8.4, -8.4, -1.9, 2.3, -0.8, -8.5, 0.2, 0.5, -7.4, 2.1, -3.9])                                                                                                                                 |   32     1     33  0.0s

[mkitti]

Wait... why is sum failing?

y = ChainedVector([[-8.5, -1.2, -3.8, 7.5], [8.2, 7.5, -5.3], [-2.7, 0.6, -6.2, 6.1, 1.4]])
x = collect(y)

julia> sum(y) == sum(x)
false

Should mapreduce be defined like this?

SentinelArrays.jl/src/chainedvector.jl

Line 784 in d13560a

Base.mapreduce(f::F, op::OP, x::ChainedVector) where {F, OP} = reduce(op, (mapreduce(f, op, y) for y in x.arrays))

Why not just use Iterators.flatten?

julia> Base.mapreduce(f::F, op::OP, x::ChainedVector) where {F, OP} = mapreduce(f, op, Iterators.flatten(x.arrays))

julia> sum(y) == sum(x)
true

[nhz2]

These changes look good. I think sum is allowed to perform the reduction in any order, so it's fine if it is only approximately equal.

[mkitti]

Currently thinking about

       function Base.mapreduce(f::F, op::OP, x::ChainedVector) where {F, OP}
           init = mapreduce(f, op, first(x.arrays))
           foreach(@view x.arrays[begin+1:end]) do a
               init = mapreduce(f, op, a; init)
           end
           return init
       end

This may be another pull request though.