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

Update the main images to overview page in profiling tutorial. #3372

Merged
merged 4 commits into from
Mar 17, 2020

Conversation

qiuminxu
Copy link
Contributor

  • Motivation for features / changes
    Update the images from scalar plugin to overview page to have a better show case of our tools.

  • Technical description of changes

  • Screenshots of UI changes

  • Detailed steps to verify changes work correctly (as executed by you)

  • Alternate designs / implementations considered

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

The notebook file has a very large diff (9.8 MB) that appears to remove
the cached output cells entirely. Could you please fix the diff such
that it only includes the desired changes so that we can review it?

@qiuminxu
Copy link
Contributor Author

The cached output cells should be removed (that shouldn't be checked in). If you check reviewnb, the diff should be easy to see.
https://app.reviewnb.com/tensorflow/tensorboard/pull/3372/files/

@qiuminxu qiuminxu requested a review from wchargin March 16, 2020 22:43
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Okay, if removing the cached cells is intentional, that’s fine (and
consistent with the rest of our docs), but could you please do so in a
separate pull request? I’m not comfortable relying on a third-party
loginwalled tool to accurately represent the diff of a commit that’s
merged into the repository history: we often need to call attention to
parts of the notebook source that are hidden by ReviewNB. It also
doesn’t seem right that a PR called “Minor updates to profiling
tutorial” should contain such a large change.

@qiuminxu
Copy link
Contributor Author

Okay, I sent out another PR to clean up the output. PTAL.
#3381

@qiuminxu
Copy link
Contributor Author

Thanks for reviewing. I've rebased this PR against #3381.

@qiuminxu qiuminxu requested a review from wchargin March 17, 2020 16:43
@qiuminxu qiuminxu changed the title Minor updates to profiling tutorial. Update the main images to overview page in profiling tutorial. Mar 17, 2020
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Thank you! This is much clearer.

docs/tensorboard_profiling_keras.ipynb Outdated Show resolved Hide resolved
@qiuminxu qiuminxu merged commit ec27ef2 into tensorflow:master Mar 17, 2020
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.

None yet

3 participants