-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Handle large mutliclass better in WIT #2385
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
Conversation
|
@tolga-b please review |
tolga-b
left a comment
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.
LGTM. In future we probably want to change the visualization of confusion matrix to make it more understandable with that many classes.
|
Agreed Tolga. This PR just makes it no longer break the tool. But it could be improved with a more significant confusion matrix change. @stephanwlee please review |
| this.selected.length > 0 ? this.selected[0] : 0; | ||
| // Get the class labels of the top scoring classes from the most | ||
| // recent inference. | ||
| const labels = this.examplesAndInferences[exampleId].inferences[ |
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.
Woah, this is a bit hard to read and parse. How about something like
const currentExample = this.examplesAndInferences[exampleId];
const lastInference = currentExample.inferences[currentExample.inferences.length - 1];
// or
// const lastInference = currentExample.inferences.slice(-1)[0];
const labels = lastInference[modelIdx].slice(0, this.maxInferenceEntriesPerRun);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
| for (let modelIdx = 0; modelIdx < data.length; modelIdx++) { | ||
| let modelDataToChart = {}; | ||
| if (Object.keys(data[modelIdx]).length > | ||
| parseInt(this.maxInferenceEntriesPerRun)) { |
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.
parseInt should have a radix or you are supposed to use Number(this.maxInferenceEntriesPerRun) unless base is not 10.
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.
+1, but stronger: parseInt should have a radix, but even then Number
is preferred by Google style, because parseInt("1 2 3") is 1 while
Number("1 2 3") is NaN. (Our internal TS style guide says “must not”
use parseInt.)
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
Models with many classes (like 100) have display issues in WIT
Only show top N classes in PD plots, just like we already do in inference panel
Correctly scroll large confusion matrices instead of overflowing into area where it can't be viewed
Ran all demos to see no regressions
Ran a demo with 100 classes to verify better visuals