Fix maximumBy comparison

[basile-henry]

maximumBy for vectors does not behave like the list version:

> maximumBy (comparing snd) $zip [0..10::Int] (repeat'a')
(10,'a')
>V.maximumBy (comparing snd) .V.fromList $zip [0..10::Int] (repeat'a')
(0,'a')

This pull request fixes this issue by checking for GT rather than LT which changes the behavior in the EQ case.

[cartazio]

This change / fix constitutes a major version change semantically right? Interface stays the same I guess :)

…

On Mon, Jul 17, 2017 at 6:30 AM Basile Henry ***@***.***> wrote: maximumBy for vectors does not behave like the list version: > maximumBy (comparing snd) $ zip [0..10 :: Int] (repeat 'a') (10,'a') > V.maximumBy (comparing snd) . V.fromList $ zip [0..10 :: Int] (repeat 'a') (0,'a') This pull request fixes this issue by checking for GT rather than LT which changes the behavior in the EQ case. ------------------------------ You can view, comment on, or merge this pull request online at: #180 Commit Summary - Fix maximumBy comparison File Changes - *M* Data/Vector/Generic.hs <https://github.com/haskell/vector/pull/180/files#diff-0> (4) Patch Links: - https://github.com/haskell/vector/pull/180.patch - https://github.com/haskell/vector/pull/180.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#180>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQwtg_xDdJ_lC7lnNo9drTN9h3HVMQks5sOze5gaJpZM4OZyur> .

[basile-henry]

@cartazio I am not sure how version changes have been handle so far with the vector package. The type signature of maximumBy does not change so I'd be tempted to just call it a patch. However it is possible some use cases rely on the old implementation.
As far as I am concerned, I'd happily wait until the next major version bump for this to be added just to be on the safe side. I'll be using the patch for my own projects in the meantime.

[cartazio]

we have no issues with major version bumps, but changes in how algorithms behave sometimes should be treated soundly :) is there a difference in performance or just the case?

…

On Mon, Jul 17, 2017 at 12:16 PM, Basile Henry ***@***.***> wrote: @cartazio <https://github.com/cartazio> I am not sure how version changes have been handle so far with the vector package. The type signature of maximumBy does not change so I'd be tempted to just call it a patch. However it is possible some use cases rely on the old implementation. As far as I am concerned, I'd happily wait until the next major version bump for this to be added just to be on the safe side. I'll be using the patch for my own projects in the meantime. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#180 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQwj6_9xMvPSCoiBtKCy3dO7M-fkWQks5sO4aEgaJpZM4OZyur> .

[basile-henry]

@cartazio I don't believe there is a difference in performance between this version and the previous one. The result of a comparison does not affect how much of the vector is used: maximumBy has to visit every single element in the vector regardless of the outcome of a comparison.

I would like to point out that the behaviour of maximumBy and minimumBy for lists is a bit odd:

> maximumBy (comparing snd) $zip [0..10::Int] (repeat'a')
(10,'a')
> minimumBy (comparing snd) $zip [0..10::Int] (repeat'a')
(0,'a')

maximumBy returns the last element which is a maximum, whereas minimumBy returns the first minimum element.

I believe this comes from the way max and min are implemented by default in Data.Ord:

-- These two default methods use '<=' rather than 'compare'-- because the latter is often more expensivemax x y =if x <= y then y else x
min x y =if x <= y then x else y

max returns the second argument in case of equality, whereas min returns the first one.

[dolio]

Yeah, it does require a major bump, since it technically changes the behavior of an exported function.

Anyhow, looks good.

[glguy]

Neither the Haskell Report's maximumBy nor Vector's appear to document any promises about the behavior when two elements are different by compare to EQ. If vector package is going to make a version bump for this change, should the haddock comment include some mention of what the guaranteed behavior is?

[basile-henry]

Writing documentation about the behavior in the EQ case makes sense. Are you thinking of something along the lines of (modified version of the Data.List.maximumBy docs) :

--| The largest element of a non-empty vector with respect to the given comparison function.---- __Note__: In the case where several elements can be considered maximum, the last one is returned.

I was thinking that maybe we shouldn't document this behavior in haddock but rather only as a comment in the code. Documenting it in haddock would tell the user of the library that they can rely on a specific implementation of this edge case.

I don't think this particular implementation makes more sense than any other one. The only reason I wanted to implement it like this is so it would match the current (also unspecified) behavior of maximumBy fromData.List.

The solution would be to either document the edge case behavior in both base and vector or in neither. In the second case, the guaranty the vector package offers is to have the same behavior as base even in some unspecified (to the user of the library) cases.

I am sure similar questions about documentation and specifications have arised in the Haskell ecosystem before. I am pretty new to Haskell library contributions, so how is this usually handled?