Adds specialized formatter to histogram component. #5818
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rileyajones
reviewed
Jul 22, 2022
Comment on lines
96
to
102
| if (absNum >= LARGE_NUMBER){ | ||
| return d3LargeFormatter(x); | ||
| } | ||
| if (absNum < SMALL_NUMBER){ | ||
| return d3SmallFormatter(x); | ||
| } | ||
| return d3MiddleFormatter(x); |
There was a problem hiding this comment.
This is very nitpicky but it's a little strange that this is ordered LARGE_NUMBER, SMALL_NUMBER, MEDIUM_NUMBER.
Perhaps consider gating around a series minimums.
Suggested change
| if (absNum >= LARGE_NUMBER){ | |
| return d3LargeFormatter(x); | |
| } | |
| if (absNum < SMALL_NUMBER){ | |
| return d3SmallFormatter(x); | |
| } | |
| return d3MiddleFormatter(x); | |
| if (absNum >= LARGE_NUMBER){ | |
| return d3LargeFormatter(x); | |
| } | |
| if (absNum >= SMALL_NUMBER){ | |
| return d3MiddleFormatter(x); | |
| } | |
| return d3SmallFormatter(x); |
If taking this suggestion you might also consider renaming SMALL_NUMBER to MEDIUM_NUMBER.
bmd3k
reviewed
Jul 25, 2022
|
Factored out the formatter and added tests. |
bmd3k
reviewed
Jul 27, 2022
yatbear
pushed a commit
to yatbear/tensorboard
that referenced
this pull request
Mar 27, 2023
dna2github
pushed a commit
to dna2fork/tensorboard
that referenced
this pull request
May 1, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for features / changes
Using SI notation for histogram bins smaller than 1 causes confusion. '500m' can mean many things, not just '0.5'. Googlers see b/238492397
Technical description of changes
Replaces tick formatter with a 3 way switch. SI notation for small values, Exponential notation for large values, and natural notation for middle values.
Screenshots of UI changes
before:
after:
Detailed steps to verify changes work correctly (as executed by you)
Ran TensorBoard standalone locally and compared across a very wide range of histogram bin bucket sizes.
Alternate designs / implementations considered
It may be possible to update tensorboard/webapp/widgets/histogram/histogram_component.ts and re-use formatters from there. Did not pursue as it would require extra customization there, and this was deemed sufficiently clear.