histogram: remove backwards buckets in v1 histogram migration

[yatbear]

Remove empty buckets on both ends to avoid having backwards (left edge > right edge) buckets.

Ran the change internally: cl/407885499

Visualization change:

• before:
  
• after:
  

#histogram #tpu

[nfelt]

Thanks for taking this on! The logic itself looks good to me, just some suggestions to make it clearer.

[nfelt]

A bit simpler as reversed(range(n))

[yatbear]

Done. Thanks!

[nfelt]

This logic might read a little easier if we special-case the if start > end case instead (to return the [k, 3] array of zeros). Otherwise, it looks like lines 97-98 above are constructing the normal-case bucket lefts and bucket rights, and then it's confusing because it's defining them both to be the same thing. But that's because what it's actually handling there is a special case (of all empty buckets). The logic that handles the normal case is down below, and it has to overwrite the bucket_lefts and bucket_rights variables, which is a bit counterintuitive IMO.

E.g.

if start > end:
    # If all input buckets were empty, treat it as a zero-bucket new-style histogram.
    return np.zeros([0, 3], dtype=np.float32)
## normal flow continues here
bucket_lefts = ...
bucket_rights = ...

[yatbear]

Done.

[nfelt]

It's a bit non-obvious how the indexing works here - even being relatively familiar with this, I had to read the code here a few times to be sure it made sense and there weren't any off-by-one issues :) In particular, I would suggest a little more explanation of why we want start:end for the bucket limits, but start:end+1 for the bucket counts, for example:

# Discard empty buckets on both ends, and keep only the "inner" edges from
# the remaining buckets. Note that bucket indices range from `start` to
# `end` inclusive, but bucket_limit indices are exclusive of `end` - this is
# because bucket_limit[i] is the right-hand edge for bucket[i].
bucket_counts = bucket_counts[start : end + 1]
inner_edges = histogram_value.bucket_limit[start:end]
# Use min as the left-hand limit for the first non-empty bucket.
bucket_lefts = [histogram_value.min] + inner_edges
# Use max as the right-hand limit for the last non-empty bucket.
bucket_rights = inner_edges + [histogram_value.max]

[yatbear]

Done, thanks for improving the readability!

[nfelt]

It might be worth an additional test case using a histogram with data that has extremal values, e.g. [-1e20, 1e20], which should produce counts in the "farthest out" buckets that the legacy histogram format can generate. In particular, in that case the final bucket in the legacy histogram format is non-empty (whereas usually it's empty).

[yatbear]

Added.

[nfelt]

I might phrase this a little differently - in particular, "min" and "max" here might be easy to confuse with the legacy histogram format's actual min and max fields (which are derived from the data), but I think you're actually referring to the minimum and maximum bucket limits? (Also, I think you have -DBL_MAX and DBL_MAX swapped.)

Maybe something like:

The "old-style" format can have outermost bucket limits of -DBL_MAX and DBL_MAX,
which are problematic for visualization. We replace those here with the actual min and
max values seen in the input data, but then in order to avoid introducing "backwards"
buckets (where left edge > right edge), we first must drop all empty buckets on the
left and right ends.

[yatbear]

This is referring to https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/lib/histogram/histogram.cc#L93, when _min and _max aren't set given empty buckets, I think the limits could end up being [DBL_MAX, -DBL_MAX], but I agree that it's very confusing, adopted the suggested doc string, thanks!

[nfelt]

Ohhh right I understand now, sorry for the confusion! But yes, since in the all-empty histogram case now, we should be ignoring the legacy format's min and max anyway (since we just produce the [0, 3] empty tensor), I think the fact that those values might be [DBL_MAX, -DBL_MAX] hopefully shouldn't be an issue.

[nfelt]

Ah right, I forgot that we need to wrap this in make_summary() in two places if we do an early return here. Alternatively we could do something like:

if start > end:
    buckets = np.zeros([0, 3], dtype=np.float32)
else:
    # rest of normal codepath goes here
    buckets = np.array([...], dtype=np.float32).transpose()
return make_summary(value.tag, summary_metadata, buckets)

Up to you!

[yatbear]

Changed the sequence to avoid moving the code blocks around.

[nfelt]

Ohhh right I understand now, sorry for the confusion! But yes, since in the all-empty histogram case now, we should be ignoring the legacy format's min and max anyway (since we just produce the [0, 3] empty tensor), I think the fact that those values might be [DBL_MAX, -DBL_MAX] hopefully shouldn't be an issue.