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 augmentation gallery #2716

Merged

Conversation

jantonguirao
Copy link
Contributor

Signed-off-by: Joaquin Anton janton@nvidia.com

Why we need this PR?

Pick one, remove the rest

  • It updates the augmentation gallery notebook showing other operators (erase, slice)

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

  • What solution was applied:
    Added operators to augmentation gallery, changed the seed to pick an image that produces appealing augmentation results.
  • Affected modules and functionalities:
    Documentation
  • Key points relevant for the review:
    NA
  • Validation and testing:
    NA
  • Documentation (including examples):
    NA

JIRA TASK: [Use DALI-XXXX or NA]

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Feb 23, 2021

View / edit / reply to this conversation on ReviewNB

JanuszL commented on 2021-02-23T18:30:39Z
----------------------------------------------------------------

1) Can we move to fn API?

2) Maybe we should remove resize as it doesn't show anything in this mosaic.


jantonguirao commented on 2021-02-25T15:46:08Z
----------------------------------------------------------------

Done

JanuszL commented on 2021-02-25T17:06:46Z
----------------------------------------------------------------

I mean to get rid of the class and use the decorator if possible. Variant with define_graph is not something we want to advertise as the recommended way.

jantonguirao commented on 2021-02-26T09:12:06Z
----------------------------------------------------------------

Done

@JanuszL JanuszL self-assigned this Feb 23, 2021
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Copy link
Contributor Author

View / edit / reply to this conversation on ReviewNB

JanuszL commented on 2021-02-23T18:30:39Z

  1. Can we move to fn API?

  2. Maybe we should remove resize as it doesn't show anything in this mosaic.

Done

Copy link
Contributor Author

Done


View entire conversation on ReviewNB

Copy link
Contributor

JanuszL commented Feb 25, 2021

I mean to get rid of the class and use the decorator if possible. Variant with define_graph is not something we want to advertise as the recommended way.


View entire conversation on ReviewNB

Copy link
Contributor Author

Done


View entire conversation on ReviewNB

Signed-off-by: Joaquin Anton <janton@nvidia.com>
…on_gallery

Signed-off-by: Joaquin Anton <janton@nvidia.com>
@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

banasraf commented on 2021-02-26T14:46:45Z
----------------------------------------------------------------

  • Just a suggestion:
outs = [lables, images]
for aug in augmentations.values():
  outs += [aug(images)]
return tuple(outs)

to

return tuple([labels, images] + [aug(images) for aug in augmentations.values()])

or

outs = [labels, images]
outs += [aug(images) for aug in augmentations.values()]
return tuple(outs)

  • From what I remember this device = 'gpu' argument is not needed. The device can be inferred.
  • Also, I think in Python style, no space around = is preferred for keyword arguments, but I guess it's ok as long as it's consistent

@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

banasraf commented on 2021-02-26T14:46:46Z
----------------------------------------------------------------

inconsistency in spacing around = (device_id=0, seed=111222)


Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2113170]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2113170]: BUILD PASSED

…on_gallery

Signed-off-by: Joaquin Anton <janton@nvidia.com>
@jantonguirao
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2124227]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2124227]: BUILD PASSED

@jantonguirao jantonguirao merged commit 8c23035 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.

4 participants