Add show.model and show.layer #9

tafsiri · 2018-09-06T22:34:20Z

also cc @caisq if you have any thoughts on useful summary information to show about a model or layer.

This change is

nsthorat

Reviewable status: 0 of 13 files reviewed, 12 unresolved discussions (waiting on @tafsiri and @nsthorat)

* @param model a `tf.Model`

## show.layerlayer(container: Drawable, layer: Layer) => Promise<void>

layerlayer?

demos/mnist_internals/index.html, line 1 at r1 (raw file):

<html>

license

demos/mnist_internals/index.js, line 1 at r1 (raw file):

import * as tf from '@tensorflow/tfjs';

license

demos/mnist_internals/model.js, line 1 at r1 (raw file):

import * as tf from '@tensorflow/tfjs';

license

demos/mnist_internals/model.js, line 46 at r1 (raw file):

remove 2 of these blank lines

demos/mnist_internals/tufte.css, line 1 at r1 (raw file):

@charset "UTF-8";

license

src/show/model.ts, line 1 at r1 (raw file):

import * as tf from '@tensorflow/tfjs';

license

src/show/model.ts, line 2 at r1 (raw file):

import * as tf from '@tensorflow/tfjs';
import {Layer} from '@tensorflow/tfjs-layers/dist/engine/topology';

don't import like this, import from the public API

src/show/model.ts, line 12 at r1 (raw file):

/**
 * Renders a summary of a tf.Model. Displays a table with layer information. *

trailing *

src/show/model.ts, line 117 at r1 (raw file):

async function getLayerDetails(layer: Layer): Promise<Array<
    {name: string, stats: HistogramStats, shape: string, weight: tf.Tensor}>> {
  // TODO consider writing an async getWeights utility

what does this mean? file an issue?

src/show/tensor.ts, line 1 at r1 (raw file):

import {Tensor} from '@tensorflow/tfjs';

license

src/show/tensor.ts, line 15 at r1 (raw file):

 * @param tensor the input tensor
 */
export async function distribution(container: Drawable, tensor: Tensor) {

maybe this should be called something like valuesDistribution? At first glance I thought tensor was actually a normalized probability distribution that we're visualizing.

or some other name, wdyt?

caisq

General comments:

This is great overall. It will be very helpful for visualizing/debugging models. Thanks for putting this together!
In the Layer Details sections, I noticed the parameters of all weights of a layer (e.g., the kernel and the bias of a dense layer) are lumped together in the histograms. I don't think this is as useful as if there are separate histograms for each weight.
You have a column that says NaN count. In the vast majority of cases, there won't be any NaNs and Infinities. Maybe hide those columns unless there are NaNs and/or Infinities? This can help make the tables less visually cluttered.
You have columns for min and max. Can we also add mean and standard deviation? Those are helpful as well.
I'm the author so take this with a grain of salt: You can consider making the "health-pill" like summaries of the types of values in the weights in a way similar the TensorBoard Debugger:
https://github.com/tensorflow/tensorboard/tree/master/tensorboard/plugins/debugger
https://github.com/tensorflow/tensorboard/blob/master/tensorboard/plugins/debugger/images/tensor_values.png

Reviewable status: 0 of 13 files reviewed, 17 unresolved discussions (waiting on @tafsiri)

src/show/model.ts, line 52 at r1 (raw file):

headers

Even though dtype is usually float32, it may be other types in some cases (e.g., int32 for a future vocab layer). I think it should be fine to omit it for now, but I just want to mention this possibility so we won't be surprised in the future.

src/show/model.ts, line 53 at r1 (raw file):

  const headers = [
    'Tensor Name',

Would it be clearer and more consistent with the terminology of keras/tfjs-layers to call this "Weight Name"? "Tensor Name" might be confused with input or output tensors.

src/show/model.ts, line 54 at r1 (raw file):

'Shape'

Just double checking, if it is a scalar, does it say "Scalar" or an empty array? The former is what should be shown, for clarity.

src/show/model.ts, line 58 at r1 (raw file):

    '# Zeros',
    '# NaNs',

If you have these two items, then it makes sense to add "# Infs", encapsulating +Infinity and -Infinity.

Same for zero count and nan count.

src/show/model.ts, line 95 at r1 (raw file):

Should the return type be explicitly described here?

tafsiri

Thanks for the review and helpful comments! This is ready for re-review

@caisq:

[per weight histograms] I actually had it that way before and thought it was too overwhelming for not enough value at first, especially as I think it requires knowledge of how that weight is applied. I think we can revisit it as I'd like to explore more what are the things someone does with the knowledge from the histogram, but at the moment I find the balance between the summary of each layer and overall histogram fairly good.
[hiding zero columns] I tend to lean away from 'disappearing UI', in this case i've also found that those are some of the most useful numbers, so always having confirmation that they are 0 is nice. I have however removed one table that i realized is superfluous and should reduce clutter, see updated screenshot below.
[std dev and mean] Will look into it, but would do it in another PR. Do you think the histogram gives a decent sense for where these measures may lie?
[health pill] I'll take a look! I do find directly reading the values nice (and could look into showing percentages). Is reading them in proportion to each other (but not the non 0/inf/nan data if i'm reading the chart correctly) particularly useful? I did find it a bit hard to read at first glance.

Reviewable status: 0 of 26 files reviewed, 8 unresolved discussions (waiting on @tafsiri and @caisq)

README.md, line 238 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

layerlayer?

Done. (fixed)

demos/mnist_internals/index.html, line 1 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

license

Done here and in the other demo

demos/mnist_internals/index.js, line 1 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

license

Done here and in the other demo

demos/mnist_internals/model.js, line 1 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

license

Done here and in the other demo

demos/mnist_internals/model.js, line 46 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

remove 2 of these blank lines

Done here and in the other demo

demos/mnist_internals/tufte.css, line 1 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

license

So this is not a file I wrote but comes from another project, I have added a link to that project and its license file

src/show/model.ts, line 1 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

license

Done

src/show/model.ts, line 2 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

don't import like this, import from the public API

This type is not in the public api, so its not ideal but i couldn't find another way. (note that i just need the type info).

src/show/model.ts, line 12 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

trailing *

Fixed

src/show/model.ts, line 52 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…

headers 
Even though dtype is usually float32, it may be other types in some cases (e.g., int32 for a future vocab layer). I think it should be fine to omit it for now, but I just want to mention this possibility so we won't be surprised in the future.

Noted

src/show/model.ts, line 53 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…

Would it be clearer and more consistent with the terminology of keras/tfjs-layers to call this "Weight Name"? "Tensor Name" might be confused with input or output tensors.

Done

src/show/model.ts, line 54 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…

'Shape'
Just double checking, if it is a scalar, does it say "Scalar" or an empty array? The former is what should be shown, for clarity.

Good suggestion. Added

src/show/model.ts, line 58 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…

    '# Zeros',
    '# NaNs',
If you have these two items, then it makes sense to add "# Infs", encapsulating +Infinity and -Infinity.

Same for zero count and nan count.

Done. Good suggestion!

src/show/model.ts, line 95 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…

 {
Should the return type be explicitly described here?

Done

src/show/model.ts, line 117 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

what does this mean? file an issue?

Never mind its already done. must have been from an older state of the code. Removed

src/show/tensor.ts, line 1 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

license

Done

src/show/tensor.ts, line 15 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

maybe this should be called something like valuesDistribution? At first glance I thought tensor was actually a normalized probability distribution that we're visualizing.

or some other name, wdyt?

Done. renamed to valuesDistribution

caisq

Responding to your responses, @tafsiri

[per weight histograms] I still think lumping the values from different weights together doesn't make much sense. Maybe you can omit the histograms by default, but when the row of each weight is clicked, it'll expand out the histogram right below it? That seems to be a good balance between visual cleanliness and the need to separate values in the histograms.
[hiding zero columns] SGTM
[std dev and mean] SGTM
[health pill] Reading them in proportions is useful IMO. It has all the information as your table rows, in addition to being more visual and intuitive. The NaN/Infinity values are highlighted by salient colors (not shown in the screenshots I sent you, I'll send you another link in private). The text above the health pills will show the count of NaNs and Infinities in the same salient colors when they are present (and won't show those counts if there are no NaN/Infinity values).

Reviewable status: 0 of 26 files reviewed, 8 unresolved discussions (waiting on @nsthorat and @caisq)

tafsiri

@caisq I added a dropdown and button so that the user can select which weight histogram they want to see. For now i'm not going to go down the health pill route, I think the table is clearer since there are very few values to compare and since in most cases these will be very small fractions of the total. Thanks for the pointers and detailed review though.

Reviewable status: 0 of 27 files reviewed, 8 unresolved discussions (waiting on @nsthorat and @caisq)

caisq

Awesome! Thanks for addressing my comments.

Reviewable status: 0 of 27 files reviewed, 8 unresolved discussions (waiting on @nsthorat and @caisq)

tafsiri added 5 commits September 6, 2018 13:19

add tensor distribution vis

df007e1

add model and layer show methods

e19e820

add tests

7e1cb13

add docs

bf3e3ea

Merge branch 'master' into add-show-model

5b5d6f5

tafsiri requested a review from nsthorat September 6, 2018 22:34

nsthorat approved these changes Sep 7, 2018

View reviewed changes

caisq suggested changes Sep 7, 2018

View reviewed changes

code review fixes. add licenses. count tensor Infinities

a559099

tafsiri commented Sep 7, 2018

View reviewed changes

caisq reviewed Sep 10, 2018

View reviewed changes

tafsiri added 2 commits September 13, 2018 11:44

add ui controls for displaying histograms in show.layer

4a6fe8f

add num params to layer details. make visor a little wider

ef4ae6e

tafsiri commented Sep 13, 2018

View reviewed changes

caisq approved these changes Sep 13, 2018

View reviewed changes

tafsiri merged commit 5708fe0 into master Sep 13, 2018

tafsiri deleted the add-show-model branch February 14, 2019 23:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add show.model and show.layer #9

Add show.model and show.layer #9

tafsiri commented Sep 6, 2018 •

edited

Loading

nsthorat left a comment

caisq left a comment

tafsiri left a comment

caisq left a comment

tafsiri left a comment

caisq left a comment

Add show.model and show.layer #9

Add show.model and show.layer #9

Conversation

tafsiri commented Sep 6, 2018 • edited Loading

nsthorat left a comment

Choose a reason for hiding this comment

caisq left a comment

Choose a reason for hiding this comment

tafsiri left a comment

Choose a reason for hiding this comment

caisq left a comment

Choose a reason for hiding this comment

tafsiri left a comment

Choose a reason for hiding this comment

caisq left a comment

Choose a reason for hiding this comment

tafsiri commented Sep 6, 2018 •

edited

Loading