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

Fix condition tracing in scale_channel #1830

Merged
merged 1 commit into from
May 14, 2020
Merged

Conversation

bhack
Copy link
Contributor

@bhack bhack commented May 13, 2020

Try to fix #1829

@bot-of-gabrieldemarmiesse

@abhichou4

You are owner of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

@seanpmorgan
Copy link
Member

Thanks! Tracing the encapsulated function appears to work:
https://colab.research.google.com/drive/18QvTLJbS-21QD1tzNyP1G3gjq2SY4RjQ?usp=sharing

Would very much like the standardize the testing for tf.data.Dataset on tfa.image with #992

@bhack
Copy link
Contributor Author

bhack commented May 13, 2020

@seanpmorgan I don't know if in the original colab example, the one with tf.function, there is a know tracing issue with partial.

@WindQAQ WindQAQ self-requested a review May 13, 2020 22:27
Copy link
Member

@WindQAQ WindQAQ left a comment

Choose a reason for hiding this comment

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

Many thanks for the quick fix :-) Could we add a simple testcase about tf.data.Dataset? Thanks.

@bhack
Copy link
Contributor Author

bhack commented May 13, 2020

I think that we have a more general issue here about partial with @tf.function.
Instead about tf.data.Dataset tests what is the plan with #992?

@WindQAQ
Copy link
Member

WindQAQ commented May 13, 2020

I think that we have a more general issue here about partial with @tf.function.
Instead about tf.data.Dataset tests what is the plan with #992?

Could you give me the link to the issue about partial and tf.function? Thank you!

@bhack
Copy link
Contributor Author

bhack commented May 13, 2020

@WindQAQ

fn = tf.function(partial(equalize_image, data_format=data_format))
it works.

@tf_function
def equalize(

doesn't trace equalize_image in partial

@tf_function
def equalize_image(

it works

@bhack
Copy link
Contributor Author

bhack commented May 13, 2020

We had some previous issues with partial:
tensorflow/tensorflow#26091
tensorflow/tensorflow#26602

But seems that they have not tested entering in partial with @tf.function like in your original colab.

Copy link
Member

@WindQAQ WindQAQ left a comment

Choose a reason for hiding this comment

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

Got it! Then I think we could merge this :-) Thanks for the contribution and clarification!

@WindQAQ WindQAQ merged commit 379fe62 into tensorflow:master May 14, 2020
@bhack bhack deleted the patch-2 branch May 14, 2020 00:22
@bhack
Copy link
Contributor Author

bhack commented May 14, 2020

If you are interested I am tying to check if partial inside a decorated function is a bug/limit of the tracer at tensorflow/tensorflow#39541

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.

tfa.image.equalize is not compatible with tf.data.Dataset
5 participants