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

Migrate distort image ops #67

Merged
merged 18 commits into from
Mar 11, 2019

Conversation

WindQAQ
Copy link
Member

@WindQAQ WindQAQ commented Mar 4, 2019

Address #25. distort_image_ops* contain random_hsv_in_yiq and adjust_hsv_in_yiq. Not sure if the benchmark functions in distort_image_ops_test.py should be removed or not.

@facaiy facaiy requested review from facaiy, seanpmorgan and armando-fandango and removed request for facaiy March 4, 2019 09:18
@facaiy facaiy added the image label Mar 4, 2019
@facaiy facaiy self-assigned this Mar 4, 2019
@facaiy
Copy link
Member

facaiy commented Mar 5, 2019

Thank you, @WindQAQ . Would you mind merging the latest master branch and run make code-format? Apologies for the conflicts, as we introduced the code style check tool and reformat all codes yesterday.

@WindQAQ
Copy link
Member Author

WindQAQ commented Mar 5, 2019

Done :-)

Copy link
Member

@facaiy facaiy left a comment

Choose a reason for hiding this comment

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

Thank you!

self._adjust_saturation_in_yiq_tf(x_np, scale)


class AdjustHueInYiqBenchmark(tf.test.Benchmark):
Copy link
Member

Choose a reason for hiding this comment

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

@karmel Hi, Karmel. Do you know what's the future for Benchmark class in tf 2.0?

#include "tensorflow/core/util/work_sharder.h"
#include "tensorflow_addons/custom_ops/image/cc/kernels/adjust_hsv_in_yiq_op.h"

namespace tensorflow {
Copy link
Member

Choose a reason for hiding this comment

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

@seanpmorgan @karmel I'm wondering if we can still use tensorflow namespace? What do you think?

@WindQAQ
Copy link
Member Author

WindQAQ commented Mar 8, 2019

Similar to #53, some assertions do not raise when test_invalid_shapes runs in graph mode.

@facaiy
Copy link
Member

facaiy commented Mar 9, 2019

Because tf 2.0 is still in the developing process (and addons too), we might run into problems as no surprise, and then have to discuss how to deal with it in the best way. Most time-consuming part for review, we really appreciate your patience, Tzu-Wei !

@facaiy
Copy link
Member

facaiy commented Mar 10, 2019

Similar to #53, some assertions do not raise when test_invalid_shapes runs in graph mode.

Could you leave a TODO comment? I don't want to delay the PR, and I think we can fix it later with #53.

cc @seanpmorgan What do you think, Sean?

@WindQAQ
Copy link
Member Author

WindQAQ commented Mar 11, 2019

Added TODO comments on test_invalid_shapes and benchmark classes. I enjoy the discussion with all Tensorflow developers, and I'll try my best to make the PR better. Thank you!

@facaiy
Copy link
Member

facaiy commented Mar 11, 2019

Tzu-Wei, feel free to ping me when you get all done ( and don't forget to use make code-format before pushing your changes)

@WindQAQ
Copy link
Member Author

WindQAQ commented Mar 11, 2019

Hi @facaiy, already made requested changes!

@facaiy facaiy merged commit a4882be into tensorflow:master Mar 11, 2019
@facaiy
Copy link
Member

facaiy commented Mar 11, 2019

Many thanks, Tzu-Wei!

Squadrick pushed a commit to Squadrick/addons that referenced this pull request Mar 26, 2019
* migrate distort_image_ops

* add test for distort_image_ops

* modify BUILD file for distort_image_ops

* add tf.fucntion decorator

* fix assert regex

* clean up internal api

* update README

* fix copyright

* import *_hsv_in_yiq

* fix name scope error

* code format

* remove sessions

* fix wrong decorators

* add TODO comments

* remove tf_test_util

* clean up messy stuff
@WindQAQ WindQAQ deleted the migrate_distort_image_ops branch April 9, 2019 02:09
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.

None yet

4 participants