Add SequenceExample viewing support to What-If Tool #1513
Conversation
|
For #1502 |
| padding-top: 10px; | ||
| } | ||
| .multi-class-checkbox { | ||
| .checkbox { |
There was a problem hiding this comment.
nit: you can just use paper-checkbox probably
| }, | ||
|
|
||
| makeAsyncRequest_: function(url, thenDoFn, postData){ | ||
| var self = this; |
There was a problem hiding this comment.
Using arrow function, now, you don't need this self
| this.requestManager.request(url, postData).then(wrapperFn); | ||
| this.requestManager.request(url, postData).then(wrapperFn) | ||
| .catch(reason => { | ||
| this.exampleStatusStr = 'Request failed' |
There was a problem hiding this comment.
nit: add semi-colon at the end for consistency
| v = v[0]; | ||
| // For numeric feature values, convert the string to a number. | ||
| if (!isNaN(v)) { | ||
| v = Number(v); |
There was a problem hiding this comment.
> isNaN('a')
< true
> Number('a')
< NaN
I believe it is more kosher to use parseInt(str, 10) to cast string to number.
There was a problem hiding this comment.
I don't want to just cast ints, must handle floats too
| } | ||
| }); | ||
| } | ||
| if (v) { |
There was a problem hiding this comment.
For easier readability, can you use a separate and descriptive variable name for v? Reading between lines, it seems like v up to this line is regarded as string (or null) and we are returning oneOf [null, NaN, number, string] (string in case squeeze == false)
There was a problem hiding this comment.
clarified with vars valueList and singleValue
| // Extract the standard features from examples or context features from | ||
| // sequence examples. | ||
| const featureRoot = example.features || example.context; | ||
| const features = 'features' in example ? |
There was a problem hiding this comment.
You may need to update this condition
| value="{{examplesPath}}" class="flex-grow"> | ||
| </paper-input> | ||
| <paper-checkbox checked="{{sequenceExamples}}" class="checkbox"> | ||
| SequenceExamples |
There was a problem hiding this comment.
would you want a whitespace between two words instead of CamelCase?
There was a problem hiding this comment.
I used camel case since SequenceExample is the proto message name that this checkbox is for.
| examples = [] | ||
| updated_example_indices = set() | ||
| sprite = None | ||
| example_class = tf.train.Example |
There was a problem hiding this comment.
Unsure but first character of the class name may need to be capitalized instead of snake_case
There was a problem hiding this comment.
Seems like a conflict between style for member variables (snake) and class names (CamelCase), of which this is both. Will keep as this to make it clear its a member variable, since the name documents that it is a class, if that is ok
2 participants
With this change, WIT can display sequence examples, but it still cannot use them for inference in models, as the Classification and Regression APIs do not support them. Will unlock their use in models in future PRs.