Skip to content

Add doctests for minmum/maximum related functions - #364

Merged
Shimuuar merged 10 commits into
haskell:masterfrom
Shimuuar:mindoc
Mar 10, 2021
Merged

Add doctests for minmum/maximum related functions#364
Shimuuar merged 10 commits into
haskell:masterfrom
Shimuuar:mindoc

Conversation

@Shimuuar

Copy link
Copy Markdown
Contributor
Doctests assume current semantics as it's implemented in code. For minimum first tie
wins, for maximum last one,
I think most convincing reason to keep this semantics is that maximum/minimum
pair works in exactly same way when equal values could be distinguished:
>>> data F = F Int Char deriving Show
>>> instance Eq F where F i _ == F j _ = i == j
>>> instance Ord F where compare = comparing (\(F i _) -> i)
>>> V.maximum [F 1 'a', F 1 'b']
F 1 'b'
>>> V.minimum [F 1 'a', F 1 'b']
F 1 'a'
As a bonus it matches behavior of list functions

@lehins@Bodigrim what's your opinion on matter?

Fixes#362

P.S. @lehins you may want to pick second commit for 0.12.2.

@lehins

lehins commented Jan 26, 2021

Copy link
Copy Markdown
Contributor

I don't know about this. Semigroup instances for Max and Min have different behavior:

λ> import Data.Semigroup
λ> import qualified Data.List.NonEmpty as NE
λ> getMax $ sconcat $fmapMax$ (Arg1'a') NE.:| [Arg1'b'::ArgIntChar]
Arg1'a'
λ> getMin $ sconcat $fmapMin$ (Arg1'a') NE.:| [Arg1'b'::ArgIntChar]
Arg1'a'

PS. Thank you. I did cherry pick it.

P.S. @lehins you may want to pick second commit for 0.12.2.

Comment threadData/Vector.hs
Comment on lines +1536 to +1650
-- >>> import qualified Data.Vector as V
-- >>> V.maximum $ V.fromList [2.0, 1.0]

@lehinslehinsJan 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor improvement to show that maximum for generic vectors works with other type of vectors:

Suggested change
-- >>> import qualified Data.Vector as V
-- >>> V.maximum $ V.fromList [2.0, 1.0]
-- >>> import qualified Data.Vector.Generic as VG
-- >>> import qualified Data.Vector as V
-- >>> VG.maximum $ V.fromList ([2, 1] :: [Integer])
-- 2
-- >>> import qualified Data.Vector as VS
-- >>> VG.maximum $ VS.fromList ([1, 2, 3] :: [Int])
-- 3

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to explain this in top-level documentation of modules. Otherwise some example will show this difference and some won't.

@Shimuuar

Copy link
Copy Markdown
ContributorAuthor

You're right. List functions behave in weird way:

>>>minimum [Arg1'a', Arg1'b']
Arg1'a'>>> minimumBy compare [Arg1'a', Arg1'b']
Arg1'a'>>>maximum [Arg1'a', Arg1'b']
Arg1'a'>>> maximumBy compare [Arg1'a', Arg1'b']
Arg1'b'

master behaves in same way, and 0.12 always returns first element. Maybe reverting changes to maximum in name of reducing breakage is better... But I don't have strong opinion here.

@Shimuuar

Copy link
Copy Markdown
ContributorAuthor

I've added few more one which uses doctests: D.V.Generic.maximum, examples for maximum/minimum which involve Arg and specified uniq behavior (#214)

Wrt maximum/minimum we have following choice:

Don't change code in master.

  1. Same behavior as list analogs
  2. Breaking change for maximumBy/maxIndexBy
  3. It's possible to get either return first or last behavior for both maximum/minimum using Down. But that's a bit of hack

Revert #180

  1. No breaking changes
  2. All functions work consistently

I'm slightly lean towards keeping things as they're in master

They assume current semantics as it's implemented in code. For minimum first tie
wins, for maximum last one,
I think most convincing reason to keep this semantics is that maximum/minimum
pair works in exactly same way when equal values could be distinguished:
>>> data F = F Int Char deriving Show
>>> instance Eq F where F i _ == F j _ = i == j
>>> instance Ord F where compare = comparing (\(F i _) -> i)
>>> V.maximum [F 1 'a', F 1 'b']
F 1 'b'
>>> V.minimum [F 1 'a', F 1 'b']
F 1 'a'
As a bonus it matches behavior of list functions
Also add description and example on whether first or last tie wins
This reverts commit 2ae8a57.
See discussion in haskell#362
@Shimuuar

Copy link
Copy Markdown
ContributorAuthor

I've reverted #180 as was discussed in #362 and added notice about difference with Data.List.maximumBy. I think PR is finally ready to merge

Comment threadData/Vector/Generic.hs Outdated
Comment threadData/Vector/Primitive.hs Outdated
Comment threadtests/Tests/Vector/Property.hs Outdated

@lehinslehins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Comment threadData/Vector/Unboxed.hs Outdated
-- ==== __Examples__
--
-- >>> import qualified Data.Vector.Unboxed as VU
-- >>> import qualified Data.Vector.Generic as VG

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-- >>> import qualified Data.Vector.Generic as VG

Comment threadData/Vector/Unboxed.hs Outdated
Comment on lines +1412 to +1413
-- >>> VG.maximum $ VU.fromList [2.0, 1.0]
-- 2.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-- >>> VG.maximum $ VU.fromList [2.0, 1.0]
-- 2.0

Seems like a copy paste error, right?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly. I don't want to dig through commit to find out, Fixed

Comment threadtests/Tests/Vector/Property.hs Outdated
@Shimuuar

Copy link
Copy Markdown
ContributorAuthor

I've addressed everything I thing and squashed few commits together

@BodigrimBodigrim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@Shimuuar
Shimuuar merged commit 4b934f9 into haskell:masterMar 10, 2021
@Shimuuar
Shimuuar deleted the mindoc branch March 10, 2021 16:49
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Semantics for maximumOn & friends

3 participants

@Shimuuar@lehins@Bodigrim