-
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 WIT ability to consume arbitrary prediction-time information #2660
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.
Thank you James, looks good to me except for minor changes.
@@ -3266,6 +3266,10 @@ <h2>Show similarity to selected datapoint</h2> | |||
observer: 'newInferences_', | |||
value: () => ({}), | |||
}, | |||
extraOutputs: { |
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.
Can you add some comments here like you did with attributions below.
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
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.
Would it be possible to define the type using closure type syntax? It does not have to be correct but it is for readability.
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.
I've added @type {indices: Array, extra: Array<{Object}>} but am not too familiar with closure.
If I want to specify that the Object above is a dict with arbitrary keys where each value in the dict is an array of numbers or strings, can I do that 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.
Yup, TypeScript types are all expressable in Closure and vice a versa. I believe you can do !Object<string, number>
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.
thx done
newExtraOutputs_: function(extraOutputs) { | ||
// Set attributions from the extra outputs, if available. | ||
const attributions = []; | ||
for (let i = 0; i < extraOutputs.extra.length; i++) { |
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.
i -> modelNum would be better here.
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
@stephanwlee trying to get a review, thx |
refreshSelectedDatapoint_: function() { | ||
const temp = this.selectedExampleAndInference; | ||
this.selectedExampleAndInference = null; | ||
this.selectedExampleAndInference = temp; |
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.
This may cause two cycles of renders. Instead, would this.set('selectedExampleAndInference', this.selectedExampleAndInference)
work? Polymer does not have any shallow equals check to shortcircuit a render.
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.
just checked. this.set('selectedExampleAndInference', this.selectedExampleAndInference) does not cause the actual visual update
@@ -3266,6 +3266,10 @@ <h2>Show similarity to selected datapoint</h2> | |||
observer: 'newInferences_', | |||
value: () => ({}), | |||
}, | |||
extraOutputs: { |
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.
Would it be possible to define the type using closure type syntax? It does not have to be correct but it is for readability.
wit.attributions = {{indices: wit.inferences.indices, | ||
attributions: inferences.attributions}}; | ||
wit.extraOutputs = {{indices: wit.inferences.indices, | ||
extra: inferences.extra_outputs}}; |
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.
indentation looks a bit off: -1 whitespace?
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
Previously, when using a custom predict function, WIT could consume attribution information for each example along with the standard prediction outputs. This work extends this so that any prediction-time information can be provided to WIT, beyond just attribution values.
Pass dictionary of extra outputs from custom predict models from python to javascript, instead of just passing attribution information. Attribution information would be inside this dict.
Update python logic to handle this dictionary correctly.
Update javascript logic for notebook mode to pass this extra information to WIT.
Update WIT to consume this information and update its visdata and examplesAndInferences structs with this extra data for each example when provided, for display in Facets Dive and the example viewer.
Extract attributions from this extra output and have WIT consume it in the same way as before.
Showing an extra field named "thingy", set in the code, displayed in the selected example, and used as an axis in the datapoints display to the right:
Run notebook with attribution to ensure attributions still handled correctly.
Run notebook with extra model output and verify that the extra information is displayed correctly both in the example viewer and Facets Dive.