Add global partial dependence plots to What-If Tool #1604
Conversation
|
@tolga-b please review |
tolga-b
left a comment
tolga-b
left a comment
There was a problem hiding this comment.
Something seems to be broken. In UCI census demo, when I click on Global PDP the browser locks up for a while then throws:
WebGL: CONTEXT_LOST_WEBGL: loseContext: context lost demo.html:5303 Uncaught (in promise) Error: Failed to compile fragment shader. at createFragmentShader (demo.html:5303) at e.createProgram (demo.html:5303) at compileProgram (demo.html:5303) at demo.html:5303 at e.getAndSaveBinary (demo.html:5303) at e.compileAndRun (demo.html:5303) at e.concat (demo.html:5303) at ENV.engine.runKernel.a (demo.html:5303) at e.runKernel (demo.html:5303) at concat2Tensors (demo.html:5303)
If this is about the number of mutants we have to spawn for all examples at once, I wonder if we somehow can do it in the background and make "Global" button available once all samples are returned instead of locking the GUI.
| <div class="main-vertical"> | ||
| <div class="main-bottom-bar"> | ||
| <div class="datapoint-left-controls-holder"> | ||
| <div class="button-prefix-label">Partial Depdependence Plots:</div> |
There was a problem hiding this comment.
Partial Depdependence -> Partial Dependence
| const inferenceAbsErrorStr = ' Inference absolute error'; | ||
| const inferenceSquaredErrorStr = ' Inference squared error'; | ||
| const inferenceScoreStr = ' Inference score '; | ||
| const inferenceScoreStr = ' Inference score'; |
| # allow for computation of global PD plots when not all examples have | ||
| # the same number of feature values for a feature. | ||
| mutant_examples.append(copied_example) | ||
| pass |
| mutant_features, inference_result_proto.result.classifications): | ||
| for idx, classification in enumerate( | ||
| inference_result_proto.result.classifications): | ||
| mutant_feature = mutant_features[idx % len(mutant_features)] |
There was a problem hiding this comment.
Could you explain the modulo here?
There was a problem hiding this comment.
Added explanatory comment
There was a problem hiding this comment.
ok now really done :)
|
thanks @tolga-b . Fixed the perf issues with global PD plots in the demos by batching of tf.js requests and constructing smaller tensors. Also made async so that UI isn't frozen and the spinner appears for the PD plots while generating, just as it does with the non-demo version already. |
| series[key] = {} | ||
| if not mutant_feature.mutant_value in series[key]: | ||
| series[key][mutant_feature.mutant_value] = [] | ||
| series[key][mutant_feature.mutant_value].append(classification_class.score) |
There was a problem hiding this comment.
this line seems to be over the width limit
| mutant_features, inference_result_proto.result.classifications): | ||
| for idx, classification in enumerate( | ||
| inference_result_proto.result.classifications): | ||
| mutant_feature = mutant_features[idx % len(mutant_features)] |
|
@stephanwlee tolga has approved the changes, can you take a look so i can merge? thanks! |
| ex.features.feature[e.detail.feature_name].bytesList.value[0] = | ||
| btoa(this.$.dash.partialDepPlotEligibleFeatures[ | ||
| featureMapping[e.detail.feature_name]].samples[i]); | ||
| const method = async () => { |
There was a problem hiding this comment.
I don't think this method has to be async since it seems to be just a callback to setTimeout. Just curious, what is the intention?
There was a problem hiding this comment.
it is async because it uses await inside. and it all is inside a setTimeout so that the UI doesn't freeze up and the display will correctly show a spinning loader while the chart is generated, as it does when not in demo mode already.
| const input = tensors.reshape([indices.length, 105]) | ||
| const res = this.model.predict(input, {batchSize: 128}); | ||
| const predValues = await res.data(); | ||
| tensors.dispose(); |
There was a problem hiding this comment.
tensors is null when indices.length is 0.
There was a problem hiding this comment.
added if check around this
| inferences.indices = indices; | ||
| inferences.results = [{regressionResult: {regressions: []}}]; | ||
| const tensors = []; | ||
| let tensors = null; |
There was a problem hiding this comment.
Correct me if I am wrong but it seems easier to parse and read to create an array and use tf.concat like this:
const tensorArr = indices.map(idx => this.convertExToTensor(this.data[idx]));
const tensors = tf.concat(tensorArr);
// Does concatting create a new tensor? Is it okay to dispose it here?
tensorArr.forEach(tensor => tensor.dispose());
const input = tensors.reshape([indices.length, 105]);There was a problem hiding this comment.
good suggestion. changing.
| } | ||
| } | ||
| const input = tf.concat(tensors).reshape([tensors.length, 105]) | ||
| const input = tensors.reshape([indices.length, 105]) |
There was a problem hiding this comment.
nit: add semi-colon at the end for consistency
There was a problem hiding this comment.
done here and in iris demo as well
| for (let idx = i; | ||
| idx < Math.min(i + BATCH_SIZE, examples.length); | ||
| idx++) { | ||
| const tensor = |
There was a problem hiding this comment.
this scoped variable is not being used.
There was a problem hiding this comment.
fixed in all demos
| idx < Math.min(i + BATCH_SIZE, examples.length); | ||
| idx++) { | ||
| const tensor = | ||
| tlist.push(this.convertExToTensor(examples[idx])); |
There was a problem hiding this comment.
idea: it would be an awesome addition if we can add this.convertExamplesToTensors(iterator) where the iterator returns an example to avoid some of these repetitions.
There was a problem hiding this comment.
i have a plan to consolidate the logic in the demos for less repeated code as it is getting to be a problem. was planning on saving it for a clean-up PR
|
|
||
| .pd-no-features-text { | ||
| font-size: 18px; | ||
| color: #777; |
There was a problem hiding this comment.
stylistic nit: css property names should be ordered alphabetically a-z here and below
There was a problem hiding this comment.
do you have a tool you use to auto-sort your CSS rules inside of polymer elements?
| }, | ||
| // True if showing global PD plots. False if showing individual PD | ||
| // plots. | ||
| globalPdPlots: Boolean, |
There was a problem hiding this comment.
should this be a readonly property?
| <div class="button-prefix-label">Partial Dependence Plots:</div> | ||
| <div> | ||
| <paper-button class="control-button" disabled$="[[!areExamplesEditable_(modelName, inferenceAddress)]]" | ||
| on-click="showGlobalPartialDependencePlots_" alt="show global partial dependence plots"> |
There was a problem hiding this comment.
consider using on-tap which works also with touch devices :)
There was a problem hiding this comment.
didn't realize this. did a global search and replace
| Args: | ||
| example_proto: The example to mutate. | ||
| example_proto: The examples to mutate. |
There was a problem hiding this comment.
example_proto -> example_protos
3 participants
Implementation for issue #1593.