-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Only open first two panes by default #871
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a creative solution to an important performance problem!
And yes, not rendering things outside the viewport seems like a great task to do later.
@@ -160,6 +160,10 @@ <h3>No audio data was found.</h3> | |||
}); | |||
}, | |||
|
|||
_shouldOpen(index) { | |||
return index <= 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this should be 1, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprisingly enough, the indexes appear to start at 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks for the info!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indexes actually start at 0: https://stackoverflow.com/questions/31388072/polymer-1-0-dom-repeat-display-index-starting-at-1
The reason that <= 2 works is because the 0th pane is the search pane, apparently:
https://github.com/tensorflow/tensorboard/blob/master/tensorboard/components/tf_categorization_utils/categorizationUtils.ts#L104
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that makes sense!
@@ -245,6 +245,10 @@ <h3>No scalar data was found.</h3> | |||
return _smoothingWeight > 0; | |||
}, | |||
|
|||
_shouldOpen(index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also update tf-pr-curve-dashboard.html
and tf-text-dashboard.html
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
This will prevent TensorBoard from using too many resources for anyone with a large number of tags. In the near future we might want to solve this in a smarter way, possibly by not rendering things outside the viewport. See also tensorflow#728
a5dcd48
to
9d1567a
Compare
This will prevent TensorBoard from using too many resources for anyone with a
large number of tags. In the near future we might want to solve this in a
smarter way, possibly by not rendering things outside the viewport.
See also #728