Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

timeseries: add option for monotonic x #4696

Merged
merged 2 commits into from
Feb 23, 2021

Conversation

stephanwlee
Copy link
Contributor

One of perennial problem of TensorBoard is zig-zaggy line chart. This
happens when a directory for a run contains event files with overlapping
step numbers in which case, TensorBoard will try to read them in order
then sort them by wall time. Our line chart, being true to the data we
are getting, does not have any special logic to break decreases in x
and renders a zig-zags which often confuses users.

This change adds a setting in timeseries to let user break such time
series into a fragment where we guarantee monotonic increases in x
within a line segment.

One of perennial problem of TensorBoard is zig-zaggy line chart. This
happens when a directory for a run contains event files with overlapping
step numbers in which case, TensorBoard will try to read them in order
then sort them by wall time. Our line chart, being true to the data we
are getting, does not have any special logic to break decreases in `x`
and renders a zig-zags which often confuses users.

This change adds a setting in timeseries to let user break such time
series into a fragment where we guarantee monotonic increases in `x`
within a line segment.
Copy link
Contributor

@psybuzz psybuzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, minor comments and curious about tooltip vs matTooltip.

<mat-icon
class="info"
svgIcon="help_outline_24px"
matTooltip="Non-monotonic steps can occur when reusing logdir with multiple summary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought our previous experiences with matTooltip (observation of bugs where it failed to reposition/disappear) led us to prefer using the web platform's .tooltip on HTMLElements instead. Are there good reasons to re-introduce matTooltip, given that this will be inconsistent with other .tooltips in the UI?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

observation of bugs where it failed to reposition/disappear

IIRC, that was not the issue. Issue was that when multiple popovers are involved, there was a significant delay and performance problem. Because we are using other kinds of popver (to name a few: modal and menu), I don't think we can workaround it all the time.

In the timeseries main content, yes, I think it is a good idea not to do without the matTooltip but I don't think we precluded it from everywhere else. In this case, with its long text, title simply are too illegible (at least on my DPI) so I very much favor using matTooltip (but it is not all that better; by default, it sets fontSize to 10px).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that said, I realized recent Chrome version has made the title much more salient than matTooltip so I replaced it (tho... the delay is a bit ugly).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for switching to title, I definitely prefer having one type of tooltip rather than 2. TB does have modals and menus, but imo they are justified in having a different appearance because their behavior is different from that of tooltips (they usually require clicks to open/close rather than mouse position movement).

(btw I just realized i wrote tooltip instead of title, my bad!)

<mat-checkbox
[checked]="scalarPartitionX"
(change)="scalarPartitionXToggled.emit()"
>Partition non-monotonic X axis</mat-checkbox
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm struggling to come up with better phrasing, but I'm wondering if we might be able to get away with slightly a longer visible label that explains how the partition will occur, without always having to read the full tooltip. Maybe something like:

"Partition non-monotonic time series into multiple monotonic series"
?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep the terse phrasing at least for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants