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

Rewrite image processing examples to fn api. #2745

Merged

Conversation

banasraf
Copy link
Collaborator

@banasraf banasraf commented Mar 1, 2021

Why we need this PR?

  • Refactoring to move our documentation to functional API

TODO:

  • BrightnessContrast Example
  • Color Space Conversion
  • HSV Example
  • WarpAffine
  • Image Decoder Examples

What happened in this PR?

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

  • What solution was applied:
    Rewriting code snippets and adjusting descriptions
  • Affected modules and functionalities:
    Image processing examples
  • Key points relevant for the review:
    Functional API usage and description
  • Validation and testing:
    N/A
  • Documentation (including examples):
    N/A

JIRA TASK: [DALI-1880]

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>
@banasraf banasraf force-pushed the move-image-processing-examples-to-fn-api branch from bc551b4 to aefa1be Compare March 1, 2021 16:53
Signed-off-by: Rafal <Banas.Rafal97@gmail.com>
@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

mzient commented on 2021-03-02T09:48:11Z
----------------------------------------------------------------

I'm 95% sure it should be

on the CPU or the GPU, depending on the device argument.


banasraf commented on 2021-03-02T13:41:10Z
----------------------------------------------------------------

done

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

mzient commented on 2021-03-02T09:48:11Z
----------------------------------------------------------------

Use max_batch_size?


banasraf commented on 2021-03-02T13:36:57Z
----------------------------------------------------------------

We don't use max_batch_size anywhere in the examples. I fell like it's kina unintuitive in this case.

@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

mzient commented on 2021-03-02T09:48:12Z
----------------------------------------------------------------

likewise


@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

mzient commented on 2021-03-02T09:48:12Z
----------------------------------------------------------------

Actually, the example is wrong - the probability argument is ignored. It should be:

sat = fn.cast(fn.random.coin_flip(probability=1-probability), types.FLOAT)

fn.hsv(images, saturation=sat)


banasraf commented on 2021-03-02T13:41:19Z
----------------------------------------------------------------

done

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

mzient commented on 2021-03-02T09:48:13Z
----------------------------------------------------------------

After adjusting the example, play with the seed to get 2 or 3 grayscale images - now only 1 got converted.


banasraf commented on 2021-03-02T13:41:39Z
----------------------------------------------------------------

done

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

mzient commented on 2021-03-02T09:48:14Z
----------------------------------------------------------------

ExternalSource -> external_source in the comment


banasraf commented on 2021-03-02T13:41:38Z
----------------------------------------------------------------

done

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

mzient commented on 2021-03-02T09:48:14Z
----------------------------------------------------------------

There's no pipeline class any more. Suggested alternative:

The processing pipeline is now defined. In order to run it, we need to instantiate and build it.


banasraf commented on 2021-03-02T13:41:41Z
----------------------------------------------------------------

done

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>
@banasraf banasraf changed the title [WPI] Move image processing examples to fn api. Move image processing examples to fn api. Mar 2, 2021
Copy link
Collaborator Author

banasraf commented Mar 2, 2021

We don't use max_batch_size anywhere in the examples. I fell like it's kina unintuitive in this case.


View entire conversation on ReviewNB

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>
@banasraf banasraf changed the title Move image processing examples to fn api. Rewrite image processing examples to fn api. Mar 2, 2021
Copy link
Collaborator Author

banasraf commented Mar 2, 2021

done


View entire conversation on ReviewNB

4 similar comments
Copy link
Collaborator Author

banasraf commented Mar 2, 2021

done


View entire conversation on ReviewNB

Copy link
Collaborator Author

banasraf commented Mar 2, 2021

done


View entire conversation on ReviewNB

Copy link
Collaborator Author

banasraf commented Mar 2, 2021

done


View entire conversation on ReviewNB

Copy link
Collaborator Author

banasraf commented Mar 2, 2021

done


View entire conversation on ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

mzient commented on 2021-03-02T14:40:36Z
----------------------------------------------------------------

Information about what this operator doesn't do is neither necessary nor welcome. We could, however, mention some formats or what the input is.

Suggested alternative:

decoders.image decodes images stored in common formats (including JPEG, JPEG2000, TIFF, PNG)


jantonguirao commented on 2021-03-02T15:12:53Z
----------------------------------------------------------------

I agree that this "no cropping" here does sound weird at this point of the tutorial.

banasraf commented on 2021-03-02T16:43:13Z
----------------------------------------------------------------

done

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

mzient commented on 2021-03-02T14:40:37Z
----------------------------------------------------------------

I think we should advertise GPU here - explain what hybrid it is and that it's accelerated, possibly by dedicated hardware.

Image Decoder (Hybrid CPU/GPU)

decoders.image with mixed backend offloads JPEG decoding to the GPU or a dedicated hardware unit, if present.


banasraf commented on 2021-03-02T16:43:18Z
----------------------------------------------------------------

done

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

mzient commented on 2021-03-02T14:40:37Z
----------------------------------------------------------------

image_slice


banasraf commented on 2021-03-02T16:43:38Z
----------------------------------------------------------------

done

Copy link
Contributor

I agree that this "no cropping" here does sound weird at this point of the tutorial.


View entire conversation on ReviewNB

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-03-02T15:20:22Z
----------------------------------------------------------------

Perhaps is not the right time to do this, but I think this second section is too repetitive. It is a copy of the first part (CPU) with the only change of the backend being "mixed". I think it doesn't add much value to the user.

That being said, this is out of the scope of this PR, we can look at that later.


banasraf commented on 2021-03-02T16:03:18Z
----------------------------------------------------------------

I totally agree, I think this notebook should be rethinked and rewritten but it's out of scope for this PR

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-03-02T15:20:23Z
----------------------------------------------------------------

no need for external source for the slice arguments, you can pass the np.array directly to slice.


banasraf commented on 2021-03-02T16:04:45Z
----------------------------------------------------------------

The idea for this example was to use external source, I think.

@review-notebook-app
Copy link

review-notebook-app bot commented Mar 2, 2021

View / edit / reply to this conversation on ReviewNB

jantonguirao commented on 2021-03-02T15:20:24Z
----------------------------------------------------------------

I'd prefer to have num_threads, device_id and such specified during instantiation rather than definition.


banasraf commented on 2021-03-02T16:42:23Z
----------------------------------------------------------------

done

@jantonguirao jantonguirao self-requested a review March 2, 2021 15:30
Copy link
Collaborator Author

banasraf commented Mar 2, 2021

I totally agree, I think this notebook should be rethinked and rewritten but it's out of scope for this PR


View entire conversation on ReviewNB

Copy link
Collaborator Author

banasraf commented Mar 2, 2021

The idea for this example was to use external source, I think.


View entire conversation on ReviewNB

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>
Copy link
Collaborator Author

banasraf commented Mar 2, 2021

done


View entire conversation on ReviewNB

3 similar comments
Copy link
Collaborator Author

banasraf commented Mar 2, 2021

done


View entire conversation on ReviewNB

Copy link
Collaborator Author

banasraf commented Mar 2, 2021

done


View entire conversation on ReviewNB

Copy link
Collaborator Author

banasraf commented Mar 2, 2021

done


View entire conversation on ReviewNB

@banasraf
Copy link
Collaborator Author

banasraf commented Mar 2, 2021

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2124114]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [2124114]: BUILD PASSED

@banasraf banasraf merged commit a0f2e16 into NVIDIA:master Mar 3, 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.

None yet

4 participants