Skip to content

Solarize_ops for Tensorflow Addons #1340

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

Closed
wants to merge 21 commits into from
Closed

Solarize_ops for Tensorflow Addons #1340

wants to merge 21 commits into from

Conversation

vinayvr11
Copy link

@vinayvr11 vinayvr11 commented Mar 19, 2020

See #1333

solarize_ops for image processing to add in Tensorflow Addons. Contains:

  1. solarize
  2. solarize_add

@abhichou4
Copy link
Contributor

It would be nice to have tests for each op. Maybe add an image like #1338

@vinayvr11
Copy link
Author

It would be nice to have tests for each op. Maybe add an image like #1338

Sure

@vinayvr11
Copy link
Author

@abhichou4 : Shall i need to test the images with all dtypes

@vinayvr11
Copy link
Author

I am getting some errors @abhichou4 could you please help me out with this.

@abhichou4
Copy link
Contributor

abhichou4 commented Mar 19, 2020

I am getting some errors @abhichou4 could you please help me out with this.

Have a look at contribution guidelines and add the pre-commit hooks. Also, BUILD and init.py need to be changed.

@vinayvr11
Copy link
Author

I am getting some errors @abhichou4 could you please help me out with this.

Have a look at contribution guidelines and add the pre-commit hooks. Also, BUILD and init.py need to be changed.

@abhichou4 : Could you please review my commit.

"interpolate_spline.py",
"connected_components.py",
"resampler_ops.py",
"solarize_ops.py",
Copy link
Contributor

Choose a reason for hiding this comment

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

compose_ops.py is missing here.

@abhichou4
Copy link
Contributor

abhichou4 commented Mar 19, 2020

I am getting some errors @abhichou4 could you please help me out with this.

Have a look at contribution guidelines and add the pre-commit hooks. Also, BUILD and init.py need to be changed.

@abhichou4 : Could you please review my commit.

Running tools/pre_commit.sh will fix formatting fails. Please confirm if your tests work with pytest.
Also, change your tests in accordance with #1328

@vinayvr11
Copy link
Author

I am getting some errors @abhichou4 could you please help me out with this.

Have a look at contribution guidelines and add the pre-commit hooks. Also, BUILD and init.py need to be changed.

@abhichou4 : Could you please review my commit.

Running tools/pre_commit.sh will fix formatting fails. Please confirm if your tests work with pytest.
Also, change your tests in accordance with #1328

Actually i am using windows so that command was not working it shows error. So you have any other options like pylint

@bhack
Copy link
Contributor

bhack commented Mar 20, 2020

Actually i am using windows so that command was not working it shows error. So you have any other options like pylint

You are on Windows you need to run that in Docker.

I've prepared a devcontainer for Vscode ide+Vscode remote extension users but It Is still not merged. It could simplify the Dev environment also on Windows. You could give a try Is here:

#1309

@vinayvr11
Copy link
Author

vinayvr11 commented Mar 20, 2020

Actually i am using windows so that command was not working it shows error. So you have any other options like pylint

You are on Windows you need to run that in Docker.

I've prepared a devcontainer for Vscode ide+Vscode remote extension users but It Is still not merged. It could simplify the Dev environment also on Windows. You could give a try Is here:

#1309

Thank you very much @bhack because for now i am using three commands separately black , pylint , flake8. So i need a way to do this fast. Thank you.

@Squadrick Squadrick self-assigned this Mar 25, 2020
Copy link
Member

@Squadrick Squadrick left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, a few changes needed.

Also, please update .github/CODEOWNERS.

@@ -1,25 +1,28 @@
licenses(["notice"]) # Apache 2.0
Copy link
Member

Choose a reason for hiding this comment

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

This is not how the BUILD is formatted. Please look at Development tips to know how to auto-format your code.

If you're on Windows, you can use docker to do the same.

@@ -0,0 +1,30 @@
""" This module is used to invert all pixel values above a threshold
Copy link
Member

Choose a reason for hiding this comment

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

Missing LICENSE, see any other source file to get the license.

Comment on lines +8 to +11
"""Method to solarize the image
image: input image
threshold: threshold value to solarize the image
"""
Copy link
Member

Choose a reason for hiding this comment

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

Use proper indents.

Copy link
Member

Choose a reason for hiding this comment

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

Look at other source files to see how docstrings are written.

import tensorflow as tf


def solarize(image, threshold=128):
Copy link
Member

Choose a reason for hiding this comment

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

Missing type.

Comment on lines +19 to +23
"""Method to add solarize to the image
image: input image
addition: addition amount to add in image
threshold: threshold value to solarize the image
"""
Copy link
Member

Choose a reason for hiding this comment

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

Indent properly.

Copy link
Member

Choose a reason for hiding this comment

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

Look at other source files to see how docstrings are written.

# we add 'addition' amount to it and then clip the
# pixel value to be between 0 and 255. The value
# of 'addition' is between -128 and 128.
added_image = tf.cast(image, tf.int64) + addition
Copy link
Member

Choose a reason for hiding this comment

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

Why the cast to int64 instead of int32?

# For each pixel in the image less than threshold
# we add 'addition' amount to it and then clip the
# pixel value to be between 0 and 255. The value
# of 'addition' is between -128 and 128.
Copy link
Member

Choose a reason for hiding this comment

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

Add a check to ensure addition is between -128 and 128.

@@ -0,0 +1,30 @@
"""Test of solarize_ops"""
Copy link
Member

Choose a reason for hiding this comment

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

Missing LICENSE.

from absl.testing import parameterized


@test_utils.run_all_in_graph_and_eager_modes
Copy link
Member

Choose a reason for hiding this comment

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

See #1328

class SolarizeOPSTest(tf.test.TestCase, parameterized.TestCase):
"""SolarizeOPSTest class to test the solarize images"""

def test_solarize(self):
Copy link
Member

Choose a reason for hiding this comment

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

Add a test for solarize_add

@seanpmorgan
Copy link
Member

Actually i am using windows so that command was not working it shows error. So you have any other options like pylint

@vinayvr11 There is a section for running pre-commit with Windows. Could you expand on the error you're getting and we'll be sure to update the steps accordingly:
https://github.com/tensorflow/addons/blob/master/CONTRIBUTING.md#coding-style

@bhack
Copy link
Contributor

bhack commented Jul 23, 2020

@bhack bhack closed this Jul 23, 2020
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.

6 participants