Added linear regression to compute execution time #46
Conversation
Fixes #40, Added information to report display
| @@ -47,7 +50,10 @@ analyze ident samp = Report | |||
| , _reportBenchParams = | |||
| map (\(Parameter x) -> fromEnum x) $ benchmarkParameters ident | |||
| , _reportTimeInNanos = | |||
| totalDuration / trueNumIterations | |||
| totalDuration / trueNumIterations | |||
There was a problem hiding this comment.
Beware! Trailing whitespace here.
There was a problem hiding this comment.
At any rate, we don't need that time anymore do we? We want to replace it with the slope of the linear regression. Am I correct? (cc @qnikst )
There was a problem hiding this comment.
I think we should just keep totalDuration here. totalDuration on it's own is meaningful statistics. Sometime we may be interested in the total time of the test and do not care about the time of each measurement, for example that may happen if we want to test RTS options or different RTS optimizations.
So I'd keep the statistics and the name, but it should be just totalDuration
There was a problem hiding this comment.
I'm unsure of what you precisely mean here: should the value of _reportTimeInNanos be replaced by totalDuration, or by slope ? In the latter case, should a new record _totalDuration be created ?
There was a problem hiding this comment.
_reportTimeInNanos = _totalDuration
| @@ -25,6 +25,9 @@ data Report = Report | |||
| { _reportBenchName :: !Text | |||
| , _reportBenchParams :: [Int] | |||
| , _reportTimeInNanos :: !Double | |||
| , _linearRegressionTime :: !Double | |||
There was a problem hiding this comment.
I think this should just replace _reportTimeInNanos, that way all current tools benefit from the improvement.
There was a problem hiding this comment.
First, let me thank you for making this PR happen, I really happy to see this!
Can you address comments by @aspiwack.
If it's not had for you can you add a bit more haddocks for the linear regression and confidence fields (what is the range and what does value there mean), that would definitely help the next reader and the user of the library (though this part is optional).
| @@ -47,7 +50,10 @@ analyze ident samp = Report | |||
| , _reportBenchParams = | |||
| map (\(Parameter x) -> fromEnum x) $ benchmarkParameters ident | |||
| , _reportTimeInNanos = | |||
| totalDuration / trueNumIterations | |||
| totalDuration / trueNumIterations | |||
There was a problem hiding this comment.
I think we should just keep totalDuration here. totalDuration on it's own is meaningful statistics. Sometime we may be interested in the total time of the test and do not care about the time of each measurement, for example that may happen if we want to test RTS options or different RTS optimizations.
So I'd keep the statistics and the name, but it should be just totalDuration
|
@nbacquey actually it would be nice if you address the comments, but if you want I can take that over can care for the comment and the merge, just tell me. |
Co-Authored-By: nbacquey <[email protected]>
Uses olsRegress from Statistics.Regression, and adds additional information to the reports and their display. Results on the micro benchmark are strange : some batches have almost perfect confidence, some others are disastrous.
Should be a step in fixing issue #40