graph] Use 0 as the minimum memory/time in the color scale.

[ioeric]

When calculating the color scale for the compute time, consider all nodes in the graph instead of just the nodes in the top-level graph.

I have a graph with one single sub-graph, which consumes all the compute time (say X ms), in the top-level graph, so the color scale becomes "X ms -> X ms".

Not sure about the original intention of the top-level graph; let me know if this is the right fix.

(I don't really know typescript. Please let me know if there is more efficient way to get the extent of a map :)

[ioeric]

@chihuahua Mind having a look at this?

[wchargin]

Just passing by, but…

Please let me know if there is more efficient way to get the extent of a map

The cleanest way is probably to take advantage of the fact that Math.min(2, 4, 1, 3, 5) === 1, so

const micros = this.hierarchy.getNodeMap().map(node => node.micros);
const minMicros = Math.min(...micros);  // ES5: Math.min.apply(null, micros)
const maxMicros = Math.max(...micros);  // ES5: Math.max.apply(null, micros)

Note that Math.min() === Infinity and Math.max() === -Infinity, not null. (This is probably a good thing, but you can always guard against it with isFinite if you want the nulls.)

Just for fun…if by “more efficient” you mean not “cleaner code” but “fewest comparisons,” then you will need to compute the min and max simultaneously. This requires (3 / 2) n − 2 comparisons (for even n) and is provably optimal. Here is a simple algorithm:

1. Group the elements x₁, …, x_n into pairs: (x₁, x₂), …, (x_n−1, x_n). (If n is odd, you can create (x_n, x_n), or modify the rest of the algorithm in the evident way.)
2. Create a list X containing the smaller value from each pair, and a list Y containing the larger value from each pair. This takes n / 2 comparisons.
3. Find the minimum of X and the maximum of Y; these must be the global minimum and maximum. This takes 2 (n / 2 − 1) = n − 2 comparisons.
   But this is surely not actually worth implementing; in addition to the fact that it only saves you about n / 2 comparisons, it’s probably slower due to caching, anyway. :-)

[chihuahua]

@ioeric, could you actually break this up into 2 PRs? Thanks! :) It's a bit cleaner that way. For instance, if we later find compute time calculations to broken, we won't have to also rollback logic for naming nodes.

[ioeric]

@chihuahua Done. Pulled the strict name change into #440

[ioeric]

@wchargin Thanks for the suggestion and analysis!

It seems that using the math library would require filtering (both nodeStats and micros can be null) plus two passes of scanning. Maybe a hand-written loop isn't too bad after all?

[chihuahua]

LGTM

[chihuahua]

Could you add a comment here noting that, furthermore, the total compute time for a metanode is the sum of the compute time of all nodes within it?

[ioeric]

I thought it's the max compute time among all nodes within it? At least this is what I saw in the graph I tested with.

[chihuahua]

The logic within the _.each block below finds the max getTotalMicros throughout the graph. In turn, getTotalMicros is the sum of compute time for all nodes underneath it (for all group nodes).

tensorboard/tensorboard/plugins/graph/tf_graph_common/graph.ts

Line 574 in efffcbf

* Sum of all children if it is a Group node. Null if it is unknown.

Come to think, I believe that means we don't need a complete graph traversal at all because top-level metanodes must have greater compute time than any of its children.

Maybe we can ignore metanodes? ie,

if (node.type === NodeType.META) {
  return;
}

[chihuahua]

Actually, lets not ignore metanodes. They get colored too.

[chihuahua]

What you can simply do is iterate through the children of the root (the top-level nodes) because group nodes already include the compute times of their children (as before). I think this change could actually be reverted. Sorry, I didn't notice earlier.

[ioeric]

But we also need the minimum to calculate the extent right? Does top-level graph have this information too?

[chihuahua]

Ah, sorry. I was out for a while. I think the scale should just range from 0 to max for the sake of graphical integrity. Otherwise, the colors might vary a lot, but the range in compute time might actually be very small.

It's similar to the reason that I prefer bar charts to start at 0:
https://flowingdata.com/2015/08/31/bar-chart-baselines-start-at-zero/

[ioeric]

I don't really have strong opinion as long as it works. I'll change the code to scale from 0.

[ioeric]

Ping. As mentioned in the inline comment, I still think the minimum compute time is not calculated correctly.

[ioeric]

PTAL

[chihuahua]

I tested, and it LGTM. Thank you for the fix.