Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added average and average1 functions #318

Merged
merged 6 commits into from
Aug 13, 2020
Merged

Conversation

aleator
Copy link
Contributor

@aleator aleator commented Aug 12, 2020

Resolves #316

Checklist:

HLint

  • I've changed the exposed interface (add new reexports, remove reexports, rename reexported things, etc.).
    • I've updated hlint.dhall accordingly to my changes (add new rules for the new imports, remove old ones, when they are outdated, etc.).
    • I've generated the new .hlint.yaml file (see this instructions).

General

  • I've updated the CHANGELOG with the short description of my latest changes.
  • All new and existing tests pass.
  • I keep the code style used in the files I've changed (see style-guide for more details).
  • I've used the stylish-haskell file.
  • My change requires the documentation updates.
    • I've updated the documentation accordingly.
  • I've added the [ci skip] text to the docs-only related commit's name.

Copy link
Contributor

@hint-man hint-man bot left a comment

Choose a reason for hiding this comment

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

Do you know why your PR is still not approved? Because I chose not to approve it. But they will.

src/Relude/Extra/Foldable.hs Outdated Show resolved Hide resolved
src/Relude/Extra/Foldable.hs Outdated Show resolved Hide resolved
src/Relude/Extra/Foldable.hs Outdated Show resolved Hide resolved
src/Relude/Extra/Foldable.hs Outdated Show resolved Hide resolved
src/Relude/Extra/Foldable1.hs Outdated Show resolved Hide resolved
src/Relude/Extra/Foldable1.hs Outdated Show resolved Hide resolved
src/Relude/Extra/Foldable.hs Outdated Show resolved Hide resolved
src/Relude/Extra/Foldable1.hs Show resolved Hide resolved
@aleator
Copy link
Contributor Author

aleator commented Aug 12, 2020

I can update CHANGELOG, but it wasn't immediately obvious if I've should've created a new hypothetical version there or not.

Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

Looking good 👍 I have a few suggestions regarding further improvements, but shouldn't be too hard to fix, I hope 🙂

. foldl' (\(!total, !count) x -> (total + x, count + 1)) (0,0)
| otherwise = Just
. uncurry (/)
. foldl' (\(!total, !count) x -> (total + x, count + 1)) (0,0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rewrite this function using recursive go function with two accumulators as separate arguments. foldl' doesn't bring much here, instead, some time and space is wasted on tuple allocation, so having a tuple is redundant. It's better to write a manual simple recursive function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a quick question here. The type is (Foldable f, Fractional a) => f a -> a. Unless it goes via a toList or something, I can't really see how to manually recurse over a generic f a.

I did a casual benchmark and having bang patterns seems almost as good as just having manual recursion.

average1_h :: NonEmpty Double -> Double
average1_h (f :| ls) = go f 1 ls
 where
  go t c [] = t/c
  go !t !c (x:xs) = go (t+x) (c+1) xs 

-- vs.
average1_c :: (foldable1 f, fractional a) => f a -> a
average1_c = uncurry (/) . foldl' (\(!total, !count) !x -> (total + x, count + 1)) (0,0) 

These benchmark over 100 000 elements like this:

benchmarking average1_h
time                 538.7 μs   (509.7 μs .. 573.3 μs)
                     0.967 R²   (0.953 R² .. 0.980 R²)
mean                 562.5 μs   (537.1 μs .. 597.6 μs)
std dev              99.05 μs   (74.08 μs .. 144.2 μs)
variance introduced by outliers: 91% (severely inflated)

benchmarking average1_c
time                 612.3 μs   (570.0 μs .. 676.1 μs)
                     0.948 R²   (0.915 R² .. 0.984 R²)
mean                 615.0 μs   (590.9 μs .. 659.5 μs)
std dev              105.2 μs   (73.69 μs .. 146.5 μs)
variance introduced by outliers: 91% (severely inflated)

Even the bang patterns don't really pay out with optimizations, but they are significant if you compile without -O. Things of course may change in practise. GHC is bit fickle.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense! Thanks for exploring this area 👍

src/Relude/Extra/Foldable.hs Show resolved Hide resolved
src/Relude/Extra/Foldable.hs Show resolved Hide resolved
src/Relude/Extra/Foldable.hs Outdated Show resolved Hide resolved
@chshersh chshersh added the extra Relude.Extra label Aug 12, 2020
aleator and others added 2 commits August 12, 2020 12:37
Co-authored-by: Dmitrii Kovanikov <kovanikov@gmail.com>
@chshersh
Copy link
Contributor

I can update CHANGELOG, but it wasn't immediately obvious if I've should've created a new hypothetical version there or not.

We can update the CHANGELOG, don't worry 🙂 But if you're willing to update it by yourself, the next version will be 0.8.0.0. There's a planned milestone for the next version. Or, you can always put ## Unreleased as a title.

Co-authored-by: Dmitrii Kovanikov <kovanikov@gmail.com>
Copy link
Contributor

@chshersh chshersh left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@vrom911 vrom911 left a comment

Choose a reason for hiding this comment

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

Nice! Thank you 👍🏼

@vrom911 vrom911 merged commit 4737d5b into kowainik:master Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extra Relude.Extra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could I add average?
3 participants