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

Rework getting started #2729

Merged
merged 3 commits into from
Mar 2, 2021
Merged

Rework getting started #2729

merged 3 commits into from
Mar 2, 2021

Conversation

awolant
Copy link
Contributor

@awolant awolant commented Feb 26, 2021

Why we need this PR?

  • Refactoring to improve Getting Started docs page

What happened in this PR?

  • What solution was applied:
      • Rework Getting Started to use fn API, decorator.
      • Move old Getting Started as legacy API example*
  • Affected modules and functionalities:
    Docs

JIRA TASK: [Use DALI-1822,1871]

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@awolant
Copy link
Contributor Author

awolant commented Feb 26, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2112355]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2112355]: BUILD PASSED

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:33Z
----------------------------------------------------------------

  • DALI offers ease-of-use and flexibility ....

^^ This paragraph is a little bit convoluted. It could use some simplification. Perhaps out of the scope of this PR but wanted to mention it.

  • ... is the Pipeline

I'd add a link to the documentation on the "Pipeline" word.


awolant commented on 2021-02-26T15:23:01Z
----------------------------------------------------------------

  1. I won't be doing it in this PR.
  2. Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:34Z
----------------------------------------------------------------

"very simple pipeline for classifier" - > "very simple pipeline for a classification task "


awolant commented on 2021-02-26T15:23:09Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:34Z
----------------------------------------------------------------

Consider making pipeline_def is a link to the documentation.


awolant commented on 2021-02-26T15:25:14Z
----------------------------------------------------------------

Done, by adding new sentence with the link at the end of the paragraph.

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:35Z
----------------------------------------------------------------

Maybe "by calling simple_pipeline, which creates a pipeline instance, and calling build on it." Your call. If you decide to keep the current wording, please add "on the newly created pipeline"


awolant commented on 2021-02-26T15:26:51Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:35Z
----------------------------------------------------------------

is a list of ... -> Isn't it a tuple?

each contains a list of tensors on the CPU -> each containing a list of CPU tensors


awolant commented on 2021-02-26T15:28:13Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:36Z
----------------------------------------------------------------

Please remove this help from here, and simply add that for more info go to the Rotate documentation (with a link)


awolant commented on 2021-02-26T15:33:27Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:36Z
----------------------------------------------------------------

How is specifying fill_value=0 is better to visualize the results? Isn't it the default?


awolant commented on 2021-02-26T14:56:31Z
----------------------------------------------------------------

No, if you don't do it the default is to copy the border pixel color.

awolant commented on 2021-02-26T15:39:16Z
----------------------------------------------------------------

To be more specific, this is the quote from the doc:

If a value is not specified, the source coordinates are clamped and the border pixel is repeated.

So technically, the default values is 0.0, but unless you put it literally it is not used.

jantonguirao commented on 2021-02-26T16:08:36Z
----------------------------------------------------------------

I understand

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:37Z
----------------------------------------------------------------

The help output for rotate operation tell us that angle parameter can accept - > rotate's angle parameter can accept

examples of support operations -> examples of operations

a pipeline with random rotation -> a pipeline that rotates images by a random angle


awolant commented on 2021-02-26T15:44:43Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:37Z
----------------------------------------------------------------

This sentence doesn't make sense anymore. There's no constructor in fn API.


awolant commented on 2021-02-26T15:53:02Z
----------------------------------------------------------------

Fixed

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:38Z
----------------------------------------------------------------

Let us modify the previous random_rotated_pipeline example to use the GPU for the rotation.


awolant commented on 2021-02-26T15:53:27Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:39Z
----------------------------------------------------------------

to use GPU -> to use the GPU

input to the rotate -> "input to rotate" or "input to the rotate operation"


awolant commented on 2021-02-26T15:46:24Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:39Z
----------------------------------------------------------------

I'd remove "In all execution paths"


awolant commented on 2021-02-26T15:48:19Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:40Z
----------------------------------------------------------------

I am missing a sentence about what "mixed" means.


awolant commented on 2021-02-26T15:50:10Z
----------------------------------------------------------------

There is a sentence after the code sample that tells what does it mean for the operation to be mixed.

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:40Z
----------------------------------------------------------------

Wondering, why did it go down?


awolant commented on 2021-02-26T15:51:08Z
----------------------------------------------------------------

Different PC, maybe different version of libraries etc.

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:41Z
----------------------------------------------------------------

moving the decoding partially to the GPU -> using GPU accelerated decoding


awolant commented on 2021-02-26T15:51:32Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:42Z
----------------------------------------------------------------

example below is a quick reference - > this examples can be used as a quick reference.

but with -> but with the legacy


awolant commented on 2021-03-01T14:59:58Z
----------------------------------------------------------------

  1. Done, but in singular form.
  2. Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:42Z
----------------------------------------------------------------

Same wording points as in the other example. I won't be repeating other points listed in the other notebook, please consider those suggestions for both notebooks when applicable.


awolant commented on 2021-03-01T15:47:58Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:43Z
----------------------------------------------------------------

by calling the build function


awolant commented on 2021-03-01T15:03:55Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:43Z
----------------------------------------------------------------

get a batch


awolant commented on 2021-03-01T15:04:40Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:44Z
----------------------------------------------------------------

2 changes to the -> 2 changes to

Shuffling is performed using -> Shuffling is performed by using

When reader - > When the reader

a next image - > the next image

spot in a buffer - > spot in the buffer


awolant commented on 2021-03-01T15:05:49Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:44Z
----------------------------------------------------------------

the previous example of the ... -> our previous ...

to use GPU for rotation operator -> to use the GPU implementation of the Rotate operator


awolant commented on 2021-03-01T15:06:52Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:45Z
----------------------------------------------------------------

to use GPU - > to use the GPU


awolant commented on 2021-03-01T15:07:21Z
----------------------------------------------------------------

Done

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 26, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-02-26T14:38:46Z
----------------------------------------------------------------

This sentence was lacking in the other notebook.


jantonguirao commented on 2021-02-26T16:11:32Z
----------------------------------------------------------------

Ignore this, I see that the sentence is there

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 1, 2021

View / edit / reply to this conversation on ReviewNB

mzient commented on 2021-03-01T16:44:29Z
----------------------------------------------------------------

Nitpick: try other seeds, here the difference between the 1st row and fixed 10 degrees is not as obvious as we could wish for.


awolant commented on 2021-03-01T17:23:45Z
----------------------------------------------------------------

Done

Copy link
Contributor Author

awolant commented Mar 1, 2021

Done


View entire conversation on ReviewNB

4 similar comments
Copy link
Contributor Author

awolant commented Mar 1, 2021

Done


View entire conversation on ReviewNB

Copy link
Contributor Author

awolant commented Mar 1, 2021

Done


View entire conversation on ReviewNB

Copy link
Contributor Author

awolant commented Mar 1, 2021

Done


View entire conversation on ReviewNB

Copy link
Contributor Author

awolant commented Mar 1, 2021

Done


View entire conversation on ReviewNB

Copy link
Contributor Author

awolant commented Mar 1, 2021

Done, also updated cat to kitten in the text at the top to make sense.


View entire conversation on ReviewNB

Copy link
Contributor Author

awolant commented Mar 1, 2021

Done


View entire conversation on ReviewNB

Copy link
Contributor Author

awolant commented Mar 1, 2021

  1. Artifacts of the review tool.
  2. Done

View entire conversation on ReviewNB

Copy link
Contributor Author

awolant commented Mar 1, 2021

Done


View entire conversation on ReviewNB

@awolant
Copy link
Contributor Author

awolant commented Mar 1, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2119823]: BUILD FAILED

@awolant
Copy link
Contributor Author

awolant commented Mar 1, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2120824]: BUILD FAILED

Signed-off-by: Albert Wolant <awolant@nvidia.com>
@awolant
Copy link
Contributor Author

awolant commented Mar 1, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2120926]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2120926]: BUILD PASSED

Signed-off-by: Albert Wolant <awolant@nvidia.com>
@awolant
Copy link
Contributor Author

awolant commented Mar 2, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2122861]: BUILD STARTED

Signed-off-by: Albert Wolant <awolant@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2122861]: BUILD FAILED

@awolant
Copy link
Contributor Author

awolant commented Mar 2, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2123102]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2123102]: BUILD PASSED

@awolant awolant merged commit 3c27ca6 into NVIDIA:master Mar 2, 2021
@JanuszL JanuszL mentioned this pull request May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants