-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Update custom scalar plugin docs with info on margin plots #878
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.
A bit of an aside, but I noticed the top-level README.md has a few questions that are a little stale now that the custom scalars dashboard exists. Would be good to update those.
https://github.com/chihuahua/tensorboard/blob/1b8c53556751b4598defaf793f3f607e24c69db8/README.md#can-i-overlap-multiple-plots
https://github.com/chihuahua/tensorboard/blob/1b8c53556751b4598defaf793f3f607e24c69db8/README.md#can-i-create-scatterplots-or-other-custom-plots (you still can't create scatterplots, but you can create margin plots now!)
@@ -6,7 +6,8 @@ The *custom* scalar dashboard lets users | |||
|
|||
1. Create line charts with custom combinations of runs and tags by assigning | |||
each chart a list of regular expressions for tags. | |||
2. Lay out the dashboard in a customized way. | |||
2. Create margin plots (for visualizing confidence intervals). |
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: maybe use an unordered list? Since these are just a set of features, there's no real inherent sequence to them.
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.
The baz chart is a margin plot. Zoom in (by dragging a rectangle) or hover over | ||
points to view margin values. Lines within the margin plot may leave the | ||
visualized margins because the primary lines are smoothened across time (but the | ||
margins are not smoothened because smoothing margins might not make statistical |
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.
It seems kinda strange to smooth the primary line but not the margins. I wonder if maybe we should just make smoothing not affect margin plots at all?
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.
That makes sense! #903
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.
I think the text here still needs to be updated? There's a broken link to the old screenshot of the line going out of bounds.
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! Done.
@@ -144,6 +93,16 @@ obtained by the regular expression `r'loss.*'`. Because the 2 tags are for the | |||
same run, the 2 lines differ in markers (One uses squares, while the other uses | |||
diamonds.) to be distinct from each other. Color still encodes the run. | |||
|
|||
The baz chart is a margin plot. Zoom in (by dragging a rectangle) or hover over |
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.
Can we make the margins in the baz chart bigger to make it more obvious that it's a margin plot in the sample_code_dashboard.png shown above? I didn't realize it was a margin chart without reading this and then going to the file and zooming in.
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.
Great idea! #923
summary = sess.run(merged_summary, feed_dict={step: i}) | ||
writer.add_summary(summary, global_step=i) | ||
``` | ||
See [custom_scalar_demo.py](custom_scalar_demo.py) for an example of collecting |
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.
(Grr at GitHub for not letting me comment on the appropriate line.)
Line 83 in the new file says "The above logic produces this custom scalar dashboard." However, now that we're just referencing the demo.py file, "above logic" isn't quite right. Also, the demo.py itself doesn't produce that exact dashboard shown in the screenshot since it now shows a margin plot for losses.
I think we probably just want to change the wording to say "Example of the custom scalar dashboard UI" or something generic?
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.
Thanks for catching the outdated comments! We now use that title and show an updated screenshot.
Oh, and sorry about the long latency on this review... slipped my mind. |
Documentation for the custom scalar plugin had not described how to create margin plots. This change describes that feature and adds/replaces screenshots. We also remove the sample code from the documentation and instead link to custom_scalar_demo.py.
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
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.
Indeed, we should update the main README for TensorBoard. I will do that in a separate PR.
summary = sess.run(merged_summary, feed_dict={step: i}) | ||
writer.add_summary(summary, global_step=i) | ||
``` | ||
See [custom_scalar_demo.py](custom_scalar_demo.py) for an example of collecting |
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.
Thanks for catching the outdated comments! We now use that title and show an updated screenshot.
LGTM. Do we know how to fix googlebot? |
Because the author "name" appears to be what's triggering that @googlebot warning.
|
Also please wrap commit message paragraphs at 72 chars. Possibly less. I noticed GitHub's GUI will render them wrong possibly at a lower number. Still working on figuring out the optimal. |
Thanks @jart - where do you see the paragraphs in commit messages rendered? Mainly just curious. Also, I think googlebot was triggered by me accidentally rebasing to a very old commit and thus pushing many unrelated commits to this PR. I got rid of them via a rebase, but googlebot's message stayed. |
…w#878) Documentation for the custom scalar plugin had not described how to create margin plots. This change describes that feature and adds/replaces screenshots. We also remove the sample code from the documentation and instead link to custom_scalar_demo.py.
Documentation for the custom scalar plugin had not described how to create margin plots. This change describes that feature and adds/replaces screenshots. We also remove the sample code from the documentation and instead link to custom_scalar_demo.py.
Documentation for the custom scalar plugin had not described how to create margin plots. This change describes that feature and adds/replaces screenshots.
We also remove the sample code from the documentation and instead link to custom_scalar_demo.py.