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

Haskell_ML.Util suggestions #10

Closed
conal opened this issue Mar 9, 2018 · 5 comments
Closed

Haskell_ML.Util suggestions #10

conal opened this issue Mar 9, 2018 · 5 comments

Comments

@conal
Copy link
Collaborator

conal commented Mar 9, 2018

  • In this definition:
    splitTrnTst :: Finite 101 -> [a] -> ([a],[a])
    splitTrnTst _ [] = ([],[])
    splitTrnTst n xs =
      let n'   = length xs * (fromInteger . getFinite) n `div` 100
          trn  = take n' xs
          tst  = drop n' xs
       in (trn, tst)
    • Fun that you're taking a Finite 101 instead of Int.
    • Wouldn't a Float, Double, or Rational make more sense than a percentage here? (I don't mind, though, since the restriction to Finite 101 is cute!)
    • In my experience, when a definition has a case for [] and a general case (not just a:as), the [] case is usually unnecessary. This code seems to be an example.
    • Use splitAt from Data.List to make a one-liner.
@conal conal changed the title Haskell_ML.Util suggestions Haskell_ML.Util suggestions Mar 9, 2018
@conal conal changed the title Haskell_ML.Util suggestions Haskell_ML.Util suggestions Mar 9, 2018
@conal
Copy link
Collaborator Author

conal commented Mar 10, 2018

A couple of other suggestions for Util:

  • Replace

            cmpr xs ys = for (zipWith maxComp xs ys) $ \case
                           True  -> 1.0
                           False -> 0.0

    by

            cmpr xs ys = cond 1 0 <$> zipWith maxComp xs ys

    where cond comes from ConCat.Misc.

    Alternatively, in SEC style:

            cmpr = (result.result.fmap) (cond 1 0) (zipWith maxComp)

    where result also comes from ConCat.Misc.

  • Replace

            maxComp u v = maxIndex u == maxIndex v

    by

            maxComp = (==) `on` maxIndex

    where on comes from Data.Function.

@conal conal reopened this Mar 10, 2018
@capn-freako
Copy link
Owner

I like your choice of "result", as a stand-in for (.).
It works for me, intuitively.
My first choice would've been "consume", but then I prefer RPN-style calculators, too. ;-)

@capn-freako
Copy link
Owner

You've shown me that trick with on, before.
I fear my retention of these more elegant programming techniques isn't quite what it should be. :(
Thanks for your patience! :)

@conal
Copy link
Collaborator Author

conal commented Mar 10, 2018

Looking better!

As usual with these things, refactorings reveal more opportunities.

  • This code:

    classificationAccuracy us vs = calcMeanList $ cmpr us vs
      where cmpr    = (result.result.fmap) (cond 1 0) (zipWith ((==) `on` maxIndex))

    I see another result.result:

    classificationAccuracy = (result.result) calcMeanList cmpr
     where
       cmpr = (result.result.fmap) (cond 1 0) (zipWith ((==) `on` maxIndex))

    But now these two uses of result.result could probably be combined into one, since they're both about reaching through the us and vs arguments.

    The intent here is getting fuzzy for me. Plumbing aside, I think the crucial computation is scoring how well we're matching maxIndex. Let's define that function:

    scoreBools :: (Functor f, Foldable f) => f Bool -> Double
    scoreBools = calcMeanList . fmap (cond 1 0)

    Then

    classificationAccuracy us vs = scoreBools (zipWith ((==) `on` maxIndex) us vs)

    Optionally, in SEC form:

    classificationAccuracy = (result.result) scoreBools (zipWith ((==) `on` maxIndex))
  • Finally, the name "calcMeanList" bothers me for two reasons:

    • "calc" is a verb, indicating action/doing, and functional programming is about being, not doing. Dropping the verb leaves "meanList", which I'd change to listMean.
    • As the type signature says, the function takes any Foldable, not just lists. I'd rename it to just "mean" (hiding mean from the import of Numeric.LinearAlgebra.Static) and fix the comment or drop it altogether:
    -- | Mean value of a collection
    mean :: (Foldable f, Fractional a) => f a -> a
    mean = uncurry (/) . foldr (\e (s,c) -> (e+s,c+1)) (0,0)
    
    scoreBools = mean . fmap (cond 1 0)

@conal conal reopened this Mar 10, 2018
@conal
Copy link
Collaborator Author

conal commented Mar 10, 2018

Hah! I used a verb: "scoreBools". How about "boolScore" instead, i.e., the score (noun) of a boolean collection, rather than to score (verb) some booleans?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants