Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

metrics: Fix percentage difference row #775

Merged
merged 1 commit into from
Oct 1, 2018

Conversation

GabyCT
Copy link
Contributor

@GabyCT GabyCT commented Sep 26, 2018

Fix the percentage difference calculation for the metrics report generation.

Fixes #709

Signed-off-by: Gabriela Cervantes gabriela.cervantes.tellez@intel.com

@GabyCT
Copy link
Contributor Author

GabyCT commented Sep 26, 2018

@grahamwhaley currently, I am fixing memory-footprint.R, if you are agree with this approach, I will do the changes in the rest of the diff calculations, thanks

@grahamwhaley
Copy link
Contributor

Hi @GabyCT .
I see the formula you used gives the % diff between the two numbers using the mid-point (average). Whilst that makes sense if you want to contemplate the % diff from pov of both numbers (iyswim), I think it makes more sense for us to look at the diff from the pov of the second number - that is, what is the % difference between the second number wrt the first - not the midpoint.

To show what I mean, I threw some obvious numbers into the .json files (100/80/110).
Here are the results:

From my original bad % code:

Results noKSM KSM Units
x 100.0 100.0 Kb
y 110.0 80.0 Kb
diff -9.09 25 %

From your PR:

Results noKSM KSM Units
x 100.0 100.0 Kb
y 110.0 80.0 Kb
diff 9.52 22.22 %

That's not quite what I had in mind though. I did a mod (for ref: using a formula like this, dropping the average and the abs to:

if (length(resultdirs) == 2) {
        # This is a touch hard wired - but we *know* we only have two
        # datasets...
        diff=c("diff")
        diference = as.double(rstats[2,2]) - as.double(rstats[1,2])
#       average = (as.double(rstats[1,2]) + as.double(rstats[2,2])) / 2
        val = 100 * (diference/as.double(rstats[1,2]))
        diff[2] = round(val, digits=2)
        diference = as.double(rstats[2,3]) - as.double(rstats[1,3])
#       average = (as.double(rstats[1,3]) + as.double(rstats[2,3])) / 2
        val = 100 * (diference/as.double(rstats[1,3]))
        diff[3] = round(val, digits=2)
        rstats=rbind(rstats, diff)

        unts[3]="%"
}

which gets me:

Results noKSM KSM Units
x 100.0 100.0 Kb
y 110.0 80.0 Kb
diff 10 -20 %

which to my eyes makes a lot more sense. wdyt?

Note: oh, also, whilst there - s/diference/difference/ ;-)

@GabyCT
Copy link
Contributor Author

GabyCT commented Sep 27, 2018

@grahamwhaley , yes I saw that formula too, I got confused if we really wanted to see the + and - signs but for me that looks better, I will work on the changes and apply them in the rest of diffs that we have in the dockerfiles thanks

@GabyCT
Copy link
Contributor Author

GabyCT commented Sep 27, 2018

@grahamwhaley changes applied thanks

@GabyCT GabyCT removed the wip label Sep 27, 2018
Fix the percentage difference calculation for the metrics report generation.

Fixes kata-containers#709

Signed-off-by: Gabriela Cervantes <gabriela.cervantes.tellez@intel.com>
@@ -98,7 +98,8 @@ if (length(resultdirs) == 2) {
# This is a touch hard wired - but we *know* we only have two
# datasets...
for( i in 1:5) {
val = ((as.double(rstats[1,i]) / as.double(rstats[2,i])) * 100) - 100
diference = as.double(rstats[2,i]) - as.double(rstats[1,i])
Copy link
Contributor

Choose a reason for hiding this comment

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

yay, this is looking good to me, and generated good output in my test. Just one tiny spelling niggle....
s/diference/difference/ throughout :-)

@GabyCT
Copy link
Contributor Author

GabyCT commented Sep 28, 2018

@grahamwhaley changes applied

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

lgtm
thanks for fixing my original wonky math ;-)

@grahamwhaley
Copy link
Contributor

/test

@devimc
Copy link

devimc commented Oct 1, 2018

woow! good catch @GabyCT
lgtm

Approved with PullApprove

@chavafg chavafg merged commit 2217550 into kata-containers:master Oct 1, 2018
@GabyCT GabyCT deleted the topic/fixdiff branch April 8, 2019 18:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants