Skip to content

Conversation

@parth-p
Copy link
Contributor

@parth-p parth-p commented Jul 25, 2019

Example notebook for all the image operations currently present in the addons repository.

@parth-p parth-p requested review from WindQAQ and facaiy as code owners July 25, 2019 02:22
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@review-notebook-app
Copy link

Check out this pull request on ReviewNB: https://app.reviewnb.com/tensorflow/addons/pull/368

You'll be able to see visual diffs and write comments on notebook cells. Powered by ReviewNB.

@parth-p
Copy link
Contributor Author

parth-p commented Jul 25, 2019

I signed the CLA!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@kyleabeauchamp
Copy link
Contributor

Looking good so far. I think you might want to show the xray picture before the median filter so that folks have a before and after.

@@ -0,0 +1,728 @@
{
Copy link
Member

@seanpmorgan seanpmorgan Jul 25, 2019

Choose a reason for hiding this comment

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

Could you click the "x" on the top left of this output to remove the install logging.

Also please use

!pip install tensorflow-gpu==2.0.0-beta1

!pip install tensorflow-addons

TFA should be 0.4


Reply via ReviewNB

@@ -0,0 +1,728 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Please update this to say something like "Here is the list of image operations we'll be covering in this example"


Reply via ReviewNB

@@ -0,0 +1,728 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

We try to stay away from using anything tensorflow.python as these are private API calls. Could You use PIL instead since it's already imported?


Reply via ReviewNB

@@ -0,0 +1,728 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Likewise remove the logging info


Reply via ReviewNB

@@ -0,0 +1,728 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

This warning should go away if using tf2-beta


Reply via ReviewNB

@@ -0,0 +1,728 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Is this the expected output we want to show?


Reply via ReviewNB

@seanpmorgan
Copy link
Member

Looking good @parth-p! Few changes requested

@seanpmorgan
Copy link
Member

One more thing... could you move the file to examples/image_ops.ipynb

@WindQAQ WindQAQ added the image label Jul 29, 2019
@@ -0,0 +1,728 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

I'd support we could use tf.io.read_file directly so that we do not need to access the private API.


Reply via ReviewNB

@@ -0,0 +1,728 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Suggestions for this:

  1. Larger values of flows are better to observe the changes.
  2. I think flows are not necessary to be a tf.Variable. Using tf.convert_to_tensor or even passing numpy array directly is enough in our implementation.

Reply via ReviewNB

@WindQAQ
Copy link
Member

WindQAQ commented Jul 29, 2019

Thank you @parth-p! I leave some comments above. Don't forget to remove unused import statement (import cv2). Please check it out. Moreover, we might want to unify our examples' style. Please follow the template.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants