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.

Sign up for GitHub

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

Jump to bottom

Add ORC reader tutorial #1465

Merged
merged 10 commits into from
Jul 26, 2021
Merged

Add ORC reader tutorial #1465

merged 10 commits into from
Jul 26, 2021

Conversation

oliverhu
Copy link
Contributor

@oliverhu oliverhu commented Jun 25, 2021 •
edited
Loading

Tutorial for the ORC reader: #1372

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@oliverhu
Copy link
Contributor Author

@kvignesh1420

@oliverhu
Copy link
Contributor Author

@kvignesh1420 updated

kvignesh1420
kvignesh1420 reviewed Jun 26, 2021
View reviewed changes
docs/tutorials/orc.ipynb Show resolved Hide resolved
docs/tutorials/orc.ipynb Show resolved Hide resolved
docs/tutorials/orc.ipynb Show resolved Hide resolved
docs/tutorials/orc.ipynb Show resolved Hide resolved
docs/tutorials/orc.ipynb Show resolved Hide resolved
kvignesh1420
kvignesh1420 requested changes Jun 26, 2021
View reviewed changes
Copy link
Member

@kvignesh1420 kvignesh1420 left a comment

Choose a reason for hiding this comment

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

@oliverhu thanks for putting this together. Please check the comments for modifications.

kvignesh1420
kvignesh1420 reviewed Jun 26, 2021
View reviewed changes
docs/tutorials/orc.ipynb Show resolved Hide resolved
docs/tutorials/orc.ipynb Show resolved Hide resolved
docs/tutorials/orc.ipynb Show resolved Hide resolved
@kvignesh1420
Copy link
Member

@oliverhu I think the notebook should contain the following sections:

  • Overview
  • Setup
  • Create tensorflow_io dataset
  • Build, compile and train a model
    with relevant sub-sections. This will help the users navigate the tutorial easily. Thanks again!

@oliverhu
Copy link
Contributor Author

@kvignesh1420 updated

@oliverhu
Copy link
Contributor Author

thanks for the review!

@kvignesh1420
Copy link
Member

@oliverhu thanks for the changes.

cc: @yongtang to take a look for additional suggestions (if any).

@kvignesh1420 kvignesh1420 requested a review from yongtang June 27, 2021 16:15
@yongtang
Copy link
Member

@oliverhu I run though the notebook and it works fine. Though can you remove the outputs from the notebook? Once it is published to tensorflow.org, then user can run on their own so no need to have outputs.

@yongtang
Copy link
Member

@oliverhu It's also worth to add a little description on the dataset and the model. I can follow the notebook but user may want to know a little bit of the background when then run through steps.

@oliverhu
Copy link
Contributor Author

@yongtang sure, I thought we need to keep the output to give user some leads how the output is supposed to look like. Will update the description as well.

@oliverhu
Copy link
Contributor Author

@yongtang updated

@oliverhu
Copy link
Contributor Author

oliverhu commented Jul 8, 2021

@yongtang good to merge now? 😄

@yongtang yongtang requested review from lamberta and MarkDaoust July 12, 2021 04:24
@yongtang
Copy link
Member

Add @tensorflow/docs-team to take a look.

MarkDaoust
MarkDaoust requested changes Jul 12, 2021
View reviewed changes
Copy link
Member

@MarkDaoust MarkDaoust left a comment

Choose a reason for hiding this comment

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

Mostly LGTM to me. Just a few comments to fix.

docs/tutorials/orc.ipynb Outdated Show resolved Hide resolved
docs/tutorials/orc.ipynb Outdated Show resolved Hide resolved
docs/tutorials/orc.ipynb Outdated Show resolved Hide resolved
@oliverhu
Copy link
Contributor Author

@MarkDaoust thanks for the review, good catches! updated, pls take another look

@kvignesh1420 kvignesh1420 requested a review from MarkDaoust July 26, 2021 16:53
MarkDaoust
MarkDaoust approved these changes Jul 26, 2021
View reviewed changes
Copy link
Member

@MarkDaoust MarkDaoust left a comment

Choose a reason for hiding this comment

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

LGTM

kvignesh1420
kvignesh1420 approved these changes Jul 26, 2021
View reviewed changes
Copy link
Member

@kvignesh1420 kvignesh1420 left a comment

Choose a reason for hiding this comment

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

Thanks @oliverhu

@yongtang yongtang merged commit 0b6ee61 into tensorflow:master Jul 26, 2021
dopiera pushed a commit to Unoperate/tensorflow_io that referenced this pull request Dec 3, 2021
@oliverhu
Add ORC reader tutorial (tensorflow#1465)
ed4c669 
* Add ORC reader tutorial

* clean up notebook

* address comments

* address comments

* address comments

* address comment: remove outputs and add desc for dataset

* fix lint

* fix lint: Prefer second person instead of first person.

* address comments

* fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvignesh1420 kvignesh1420 kvignesh1420 approved these changes

@MarkDaoust MarkDaoust MarkDaoust approved these changes

@yongtang yongtang Awaiting requested review from yongtang

@lamberta lamberta Awaiting requested review from lamberta

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oliverhu @kvignesh1420 @yongtang @MarkDaoust