-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add median stats to What-If Tool regression performance stats table #2434
Conversation
@tolga-b please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good James with some minor comments.
function mean(data) { | ||
const sum = data.reduce((sum, value) => { | ||
return sum + value; | ||
}, 0); | ||
|
||
return sum / data.length; | ||
} | ||
function median(sortedData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be a style thing, but if we sorted arrays within the median function, we could get rid of some repetition in the inputs below (l3549-l3551).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
function mean(data) { | ||
const sum = data.reduce((sum, value) => { | ||
return sum + value; | ||
}, 0); | ||
|
||
return sum / data.length; | ||
} | ||
function median(sortedData) { | ||
if (sortedData.length == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mean does not have this check, is it necessary there as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mean will return NaN already due to its use of reduce with an initial value of 0. Since median uses indexing instead of a reduce fn, it needs the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Purely from the UI point of view, it looks like the header is misaligned with the content. I won't block the merge on it but consider addressing that in future changes.
@@ -5811,7 +5847,9 @@ <h2>Show similarity to selected datapoint</h2> | |||
sorts = sorts.concat(['Accuracy']); | |||
} else { | |||
sorts = sorts.concat( | |||
['Mean error', 'Mean absolute error', 'Mean squared error']); | |||
['Mean error', 'Median error', 'Mean absolute error', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like this really should be enum.
Users asked for median error stats in addition to current mean error stats
Added calculation of, and display of, median error, median absolute error, and median squared error to the regression performance table.
Ran regression demo and viewed performance stats in total and broken down into slices. Verified correct median numbers and that sorting by those new columns worked.