-
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 ability to set attributions in WIT #2252
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.
Added some comments.
@@ -397,7 +397,7 @@ Polymer({ | |||
(d: {}, i: number) => this.getColorForSaliency(val[i]) : | |||
() => this.getColorForSaliency(val); | |||
this.selectAll( | |||
`input.${this.sanitizeFeature(feat.name)}.value-pill`) | |||
`iron-autogrow-textarea.${this.sanitizeFeature(feat.name)}.value-pill`) |
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.
General comment, it would be better if this code did not depend on the type of text box (iron-autogrow-textarea or input). It looks a bit fragile right now in case we forget to change this when html side is updated.
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 code actually works correctly without the selector for the element this, so i removed it. Thanks for catching this.
@@ -122,8 +123,11 @@ def infer_impl(self): | |||
self.estimator_and_spec.get('estimator'), | |||
self.estimator_and_spec.get('feature_spec'), | |||
self.custom_predict_fn) | |||
infer_objs.append(inference_utils.run_inference_for_inference_results( | |||
(preidctions, attributions) = ( |
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.
typo
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
@@ -413,6 +413,24 @@ def set_custom_predict_fn(self, predict_fn): | |||
- For regression: A 1D list of numbers, with a regression score for each | |||
example being predicted. | |||
|
|||
Optionally, if attributions can be returned by the model with each | |||
prediction, then this method can return a dict with the key 'predictions' | |||
containing the preidctions result list described above, and with the key |
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.
typo preidctions, this happens in a lot of places so mass replace should fix it.
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 i = 0; i < attributions.indices.length; i++) { | ||
const idx = attributions.indices[i]; | ||
const datapoint = this.visdata[idx]; | ||
// For now, we only display attribution from the first model, if WIT |
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.
Add TODO here for two models
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
@@ -2186,6 +2186,27 @@ <h2>Show similarity to selected datapoint</h2> | |||
observer: 'newInferences_', | |||
value: () => ({}) | |||
}, | |||
// Attributions from inference. A dict with two fields: 'indices' and | |||
// 'attributions'. Inidicies contains a list of example indices that |
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.
typo Inidicies
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 please review thx |
infer_objs.append(inference_utils.run_inference_for_inference_results( | ||
examples_to_infer, serving_bundle)) | ||
(predictions, _) = inference_utils.run_inference_for_inference_results( | ||
examples_to_infer, serving_bundle) |
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.
+2 whitespace per python style guide.
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
attributions: { | ||
type: Object, | ||
observer: 'newAttributions_', | ||
value: () => ({}) |
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.
null
might be better.
datapoint[attribSparseValueKey] = attribs[keys[j]][1]; | ||
} else { | ||
const attribKey = attributionPrefix + keys[j]; | ||
datapoint[attribKey] = attribs[keys[j]]; |
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 feels un-Polymermic to mutate data like this and then trigger a draw (as opposed to using prop changes to trigger data binding). Can you consider creating another object and do this.set(`visdata[${index}]`, newObject)
?
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.
fixing
Enabling WIT to be able to display feature-level attributions for individual predictions if the model provides it.
Created custom predict function that created fake attributions and inferences, tested it in colab and jupyter notebooks to see correct visual handling of those attributions.