On Tue, May 04, 2010 at 04:53:55 -0400, Max Battcher wrote: > One last tweak for the night-- was missing bar spacing information. > The graphs should now look even more like the versions on the wiki.
I've gone ahead and pushed this. Hope you keep on hacking Haskell!
Unfortunately, my review isn't going to be particularly helpful as
it's (a) subjective Eric style, which may be meaningless and (b)
consists of saying "huh?" followed by the realisation that the "huh"
bits are code I wrote.
Could be a useful exercise to go ahead and try to tackle the duplication
maybe in bits and pieces.
Add GChart graphs to reports
----------------------------
> +-- A Formatter with Double output instead of String
> +type Selector = TimeUnit -> Maybe MemTimeOutput -> Double
I'm a bit skeptical about type synonyms like this that don't get used
very often.
It's sort of a delicate balance, and I never know which way ultimately
leads to more readability, but it does sometimes happen that actually
the long expanded version is more transparent. I wish I could figure
out when :-/
Heh, of course, you were just following the type synonym I wrote
but just be warned that maybe I was making a mistake :-)
> +selectMemory :: Selector
> +selectMemory _ (Just mt) = fromRational (mtMemMean mt) / (1024 * 1024)
> +selectMemory _ _ = 0.0
> +
> +selectTime :: Selector
> +selectTime Milliseconds (Just mt) = mtTimeMean mt * 1000
> +selectTime MinutesAndSeconds (Just mt) = mtTimeMean mt
> +selectTime _ _ = 0.0
What do the Nothing cases correspond to?
> +graph :: Bool -- ^ Select for Time, else Memory
> + -> [(Test a, Maybe MemTimeOutput)] -> [(String, String)]
> +graph timeresults results = map graphs coreNames
> + where
> + graphs cname = (cname, mkGraphs cname)
> + coreNames = nub [ trCoreName r | (Test _ r _, _) <- results ]
> + mkGraphs cname = graphRepo timeresults cname results
One graph set per repo (eg. tabular).
I think mkGraphs is a little superfluous and
graphs cname = (cname, graphRepo timeresults cname)
would be lighter
> +graphRepo :: Bool -- ^ Select for Time, else Memory
> + -> String -- ^ Repo name
> + -> [(Test a, Maybe MemTimeOutput)] -> String
Maybe this should return [String] and avoid not use graphDirective.
Just focus on generating the graph itself...
> +graphRepo timeresults repo results = unlines $ map (graphDirective .
> graphUrl) rs
> + where
Personal (Eric) style note: this is why you'll see my code littered with
seemingly meaningless empty "--" comment marks. I just like to break up
large blocks of helper functions into sections for my idea of
readability. YMMV. Clearly the kind of thing that could be abused.
> + -- the results which correspond to the repo in question (or a variant
> thereof)
> + interesting = [ test | test@(Test _ r _, _) <- results, trCoreName r ==
> repo ]
> + columns = nub [ (b, trName tr) | (Test _ tr b, _) <- interesting ]
> + rownames = map description . filter hasBenchmark $ benchmarks
> + hasBenchmark b = any (\ (Test tb _ _, _) -> description tb == description
> b) interesting
Huh? [This is an expression of sheepishness... I know this comes
straight from Report.hs, but looks like a bit of write-only code
on my part. Sorry!]
> + rows = [catMaybes [find (match row col) interesting | col <- columns]
> + | row <- rownames]
> + match bench (binary,name) (Test bench' tr binary', _) =
> + bench == description bench' && binary == binary' && trName tr == name
> + rs = [(title (head row) row, map mkColName columns, rowdata row)
> + | row <- rows]
I have the impression that this is really the meat of this function, but
I didn't really have much time to think about it :-(
> + mkColName (TestBinary b, tname) =
> + let v = nameToVariant tname
> + prefix = case vId v of
> + DefaultVariant -> ""
> + _-> vSuffix v ++ " "
> + in prefix ++ cutdown b
> + cutdown d | "darcs-" `isPrefixOf` d = cutdown (drop 6 d)
> + | takeExtension d == ".exe" = dropExtension d
> + | otherwise = d
You're right, these should be factored out.
Just promote them top-level functions, maybe with a longer more
specific name.
> + rowdata row = [select (tu row) mt | (_, mt) <- row]
> + title (Test b _ _, _) row = description b ++ " (" ++ unitName (tu row) ++
> ")"
> + select = if timeresults then selectTime else selectMemory
> + unitName tu = case (timeresults, tu) of
> + (True, Milliseconds) -> "ms"
> + (True, MinutesAndSeconds) -> "s"
> + (False, _) -> "MiB"
> + tu row = case timeresults of
> + True -> case mapMaybe getTime row of
> + [] -> Milliseconds
> + xs -> appropriateUnit (minimum xs)
> + False -> Milliseconds
> + getTime (_, Just mt) = Just $ mtTimeMean mt
> + getTime _ = Nothing
This sort of selection logic makes me wonder if you'd not be better off
having separate functions for memory and timing graphs. Maybe you can
divide this function up in a different way?
Presumably you tried to put it all in one function because the logic for
picking out all of the benchmarks is common to graphs. But could you
not just isolate *that* (rs) into a function of its own and then have
two functions that wrap around that instead? Do you see what I mean?
> +graphDirective :: String -> String
> +graphDirective = (++) ".. image:: "
This could be a little bit clearer using an operator section:
graphDirective = ("image:: " ++)
> +graphUrl :: (String, [String], [Double]) -> String
> +graphUrl (title, labels, results) = getChartUrl $ do
> + setChartSize 200 200
> + setDataEncoding simple
> + setChartType BarVerticalGrouped
> + setBarWidthSpacing $ barwidthspacing 23 5 20
> + setChartTitle title
> + addAxis $ makeAxis { axisType = AxisBottom, axisLabels = Just labels }
> + addAxis $ makeAxis { axisType = AxisLeft,
> + axisRange = Just $ Range (0, realToFrac range) Nothing }
> + setColors palette
> + addChartData row
> + where
> + range = case results of
> + [] -> 1
> + xs -> maximum xs
You can also write
range = if null results then 1 else maximum results
> + row = [truncate (result * 61.0 / range) | result <- results] :: [Int]
> + , "Timing Graphs"
> + , "===================================================="
Do we want all the graphs in one big block? Could work...
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
pgpupAwORv1tz.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
