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

Add ipynb tutorial for text summaries #4718

Merged
merged 5 commits into from
Mar 4, 2021

Conversation

bileschi
Copy link
Collaborator

@bileschi bileschi commented Mar 1, 2021

  • Motivation for features / changes
    Adds a tutorial for how to use text summaries. The tutorial is very basic and only covers adding text under different tags, as well as how to use markdown. An example is included for how to use markdown to nicely format a json object.

  • Technical description of changes

Added a ipynb file by generating it on my local machine and testing in jupyter.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@google-cla google-cla bot added the cla: yes label Mar 1, 2021
@bileschi bileschi requested a review from nfelt March 1, 2021 22:55
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

Thanks for creating this!

docs/text_summaries.ipynb Outdated Show resolved Hide resolved
docs/text_summaries.ipynb Outdated Show resolved Hide resolved
docs/text_summaries.ipynb Outdated Show resolved Hide resolved
docs/text_summaries.ipynb Outdated Show resolved Hide resolved
docs/text_summaries.ipynb Outdated Show resolved Hide resolved
"def pretty_json(hp):\n",
" json_hp = json.dumps(hp, indent=2)\n",
" s = \"\"\n",
" for line in json_hp.splitlines():\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit more compact and efficient to just do return "".join("\t" + line for line in json_hp.splitlines(True))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, this is better

docs/text_summaries.ipynb Outdated Show resolved Hide resolved
"TensorBorad supports a number of markdown idioms, such as\n",
"\n",
" preformatted code\n",
" \n",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra spaces on this line; I wouldn't bother to comment except this is an actual code sample so it's nice to ensure it's stylistically clean

"source": [
"## Using markdown\n",
"\n",
"Tensorboard supports logging text in markdown to make it easier to read and understand."
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, you actually don't have a choice: TensorBoard interprets all text that users log as potential Markdown. You might expect this is undesirable sometimes and that would be correct: #830

Until we fix that, I think it's perhaps more accurate to title this section ## Markdown interpretation and then say something like:

TensorBoard interprets text summaries as Markdown, since rich formatting can make the data you log easier to read and understand, as shown below. (If you don't want Markdown interpretation, see this issue for workarounds to suppress interpretation.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, yes also better, thanks.

docs/text_summaries.ipynb Outdated Show resolved Hide resolved
@bileschi bileschi requested a review from nfelt March 3, 2021 15:37
"data": {
"text/html": [
"\n",
" <iframe id=\"tensorboard-frame-9381929c3767b97b\" width=\"100%\" height=\"800\" frameborder=\"0\">\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we want to cache the output for this iframe? I wouldn't expect it will render anything useful (outside the tensorflow.org site, where I think it gets replaced by a static image), so it seems better to omit IMO.

Ditto elsewhere below where we have the iframe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed,

Aside, I've been editing this file in the jupyter lab UI. Is there an easy way to drop the cell output there, or do I need to edit somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know whether there's a way to do that in the jupyter lab UI; I am not very familiar with it. But you can always just edit the JSON file.

{
"data": {
"text/plain": [
"Reusing TensorBoard on port 6008 (pid 44187), started 1 day, 16:31:45 ago. (Use '!kill 44187' to kill it.)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably omit this cached output as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dropped all of them.

"source": [
"%tensorboard --logdir logs"
"%tensorboard --logdir logs/multiple_texts --samples_per_plugin 'text=5'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a better example to set the sample count to something higher (e.g. at least 20, since that's the step count here, or maybe something like 100). It seems a lot more likely that folks will want to increase the count than decrease it (relative to the default value of 10).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I intentionally kept 5 here to illustrate to the user that subsampling occurred. I suspect that this will encourage users to play with the parameter. If we set it higher than 20, users will probably overlook it.

@bileschi bileschi merged commit 843d231 into tensorflow:master Mar 4, 2021
@bileschi
Copy link
Collaborator Author

bileschi commented Mar 4, 2021 via email

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

Successfully merging this pull request may close these issues.

2 participants