-
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 CSV-loading mode #1597
Add CSV-loading mode #1597
Conversation
I will get this reviewed by Tolga once he is added to tensorboard-team, but feel free to take a look as well Stephan |
@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.
Adding some comments
@@ -1476,8 +1479,13 @@ <h2>Create a distance feature</h2> | |||
const inferenceErrorStr = ' Inference error'; | |||
const inferenceAbsErrorStr = ' Inference absolute error'; | |||
const inferenceSquaredErrorStr = ' Inference squared error'; | |||
const inferenceScoreStr = ' Inference score '; | |||
const inferenceScoreStr = ' Inference score'; |
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.
space in the end is intentional since we append model number directly to inferenceScoreStr when showing inference scores for multiple models now.
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.
whoops, thx
@@ -477,6 +493,11 @@ Polymer({ | |||
} | |||
min = Math.min(0, min) * clipSaliencyRatio; | |||
max = Math.max(0, max) * clipSaliencyRatio; | |||
if (min < 0 && max > Math.abs(min)) { |
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.
Could you comment on this change, why are we making values symmetric around zero?
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
from glob import glob | ||
from grpc.beta import implementations | ||
import random | ||
import numpy as np |
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.
is np used?
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.
good catch. not anymore. removed from here and build file
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.
thanks for the comments tolga, made the appropriate changes
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.
@@ -274,6 +276,20 @@ Polymer({ | |||
.getFeatureLists()!.getFeatureListMap(); | |||
}, | |||
|
|||
setFilteredFeaturesList: function(featureList: NameAndFeature[], | |||
searchValue: string, saliency: SaliencyMap) { | |||
this.filteredFeaturesList = []; |
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.
JS style nit: please -2 indent in this function block and the one 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
// for negative and positive attributions are the same, for visual | ||
// consistency. | ||
if (min < 0 && max > Math.abs(min)) { | ||
min = -1 * max; |
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.
another style nit: -1 indent before max
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
@@ -1071,7 +1074,7 @@ Polymer({ | |||
* be used in css classes/ids. | |||
*/ | |||
sanitizeFeature: function(feat: string) { | |||
let sanitized = feat; | |||
let sanitized = feat.trim(); |
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.
nit: indentation looks off
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 saying, because of L1079, sanitized
may still have the whitespace. Perhaps use sanitized
in L1078 and L1079?
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.
you're right, thx
compare-title="[[compareTitle]]"> | ||
compare-title="[[compareTitle]]" | ||
saliency="[[attribution]]" | ||
show-saliency="true" |
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.
you can omit ="true"
when specifying boolean prop.
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
@@ -2829,6 +2849,70 @@ <h2>Create a distance feature</h2> | |||
this.selectedExampleAndInference = | |||
this.selected.length > 0 | |||
? this.examplesAndInferences[this.selected[0]] : null; | |||
requestAnimationFrame(()=> this.setAttributions()); |
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.
for legibility, can you put one whitespace before =>
? thanks!
also, often, if you start a RAF, it is a good practice to cancel it on unmount or detached
. Please consider cancelling the RAF.
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
@@ -2093,6 +2108,11 @@ <h2>Create a distance feature</h2> | |||
this.isBinaryClassification_(modelType, multiClass); | |||
}, | |||
|
|||
enablePartialDependencePlots_: function(selectedExampleAndInference, modelName, inferenceAddress) { |
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.
like areExamplesEditable_
method, it might read better if this method name is isPartialDependencePlotsEnabled_
or something like this
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
*/ | ||
setAttributions: function() { | ||
const attrib = {}; | ||
const selectedData = this.visdata[this.selected[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.
stupid question: couldn't read the entire file but it roughly looks like only the first element in selected
is used. Why is it using array for its type?
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 please provide what kind of structure visdata
has in the code? thanks!
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.
selected is bound to facets-dive selected attribute, which is an array of indices of selected elements in facets-dive. in the what-if tool we only make use of the first selected element.
added more comments to visdata and selected definitions to specify structure.
If given a CSV file (with .csv extension) as the examples path, then the tool will create and visualize examples from CSV as opposed to tfrecords. Additionally, the CSV can contain columns for model inference output and per-feature attribution for a given inference - as might be provided by the TF model analysis project (TFMA) project. The attributions are visualized in the example viewer per code that already exists in that component.
In CSV mode, examples aren't editable and PD plots aren't accessible since there is no live model to infer examples against.