findmax is broken for repeated values

[mkitti]

@quinnj findmax is still broken when there are repeated values.

julia> c = ChainedVector([[7, 21, 31], [45], [53, 53]])
6-element ChainedVector{Int64, Vector{Int64}}:
  7
 21
 31
 45
 53
 53

julia> findmax(c)
(53, 6)

julia> findmax(c |> collect)
(53, 5)

Fixed by #99

[mkitti]

The problem with findmax is that !isless is not the correct way to invert isless for this purpose.

SentinelArrays.jl/src/chainedvector.jl

Line 812 in f9e11b7

return findXwithfirst(!isless, f, x, y, i)

[gdalle]

For what it's worth @mkitti, when you say on Discourse

While I am glad the original issue was fixed, I’m wondering why the attention to the package was so narrow to ignore the other pull request addesssing perhaps the deeper issue - insufficient testing. I’m sure it was an oversight due to limited time.

well indeed my "attention to the package was narrow". I had never touched or heard about this package before yesterday. I saw an issue that had an easy fix, and I fixed it. You found another one, I then asked you whether my fix resolved it too, and didn't get an answer. My take is that merging my fix improved the state of things, albeit not completely, and could be done quicker than a more ambitious refactoring of the whole test suite because review was instantaneous. I'm not saying it was a perfect solution, but it's not a worsening of the situation.
What I should have done is include your new test case in the suite, but I did not understand how it deviated from the initial one. Sorry about that.

[mkitti]

didn't get an answer

I reponded with the best information I had at the time, which was that I had incorporated your fixes into my branch. I was still focused on building out testing itself, so I was not in a position to check out your branch and test it in isolation. I do not think there was anything technically wrong with your pull request other than the statement that it fixed #97 . The description of #97 not only reported a problem with argmax but it also problem with the insufficiency of a single test vector. Eventually, I did realize, 2 hours later, that your fixes were not sufficient to address the issue I identified and then I fixed it myself.

Like you, I have not touched nor was aware of this package until yesterday.

The problem I see is that it would be very easy to see that the issue was now "fixed" and for everyone's eyes to shift elsewhere thinking everything is now all good. I'm being a bit loud about this because I'm not quite sure this will get resolved otherwise.

Overall though, I am concerned about the systemic pattern of addressing bugs narrowly in a reactive fashion rather than considering the problems with the process that permitted the issue from being detected. Shifting this thinking would allow us to be more proactive in discovering bugs.

Please do not take the critique personally. I do appreciate your self-reflection. Not long ago, I would have taken the same approach as you.

[gdalle]

I am taking notes and will try to be a little more systemic next time 😊
To be fair, my perspective is also informed by many PRs I have made or helped review, including for bug fixes, which were never merged because they were never perfect. I have found that this discourages contributors, and doesn't make the software more reliable either. So my approach now, for instance in Graphs.jl, is that an imperfect feature is better than no feature, and that an imperfect fix is better than no fix. In this spirit, I probably should not have claimed that #97 was completely solved, but I hope it helps you understand where I come from

[Moelf]

just to be clear this is still broken?

[mkitti]

Yes, the current release, 1.4.2, is still broken. It's not very hard to confirm that.

julia> c2 = ChainedVector([[42, 3, 8, 41], [23, 32], [42, 17]])
8-element ChainedVector{Int64, Vector{Int64}}:
 42
  3
  8
 41
 23
 32
 42
 17

julia> findmax(c2)
(42, 7)

julia> findmax(collect(c2))
(42, 1)

(jl_ftBePh) pkg> st
Status `/tmp/jl_ftBePh/Project.toml`
  [91c51154] SentinelArrays v1.4.2

[mkitti]

@quinnj Could you please take a look how we could fix findmax not returning the correct index? I've proposed a fix in #99.