Skip to content

Remove tf function where unecessary #823

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged

Conversation

seanpmorgan
Copy link
Member

Closes #807

Removed from:

  • Functions that are simple calls to a custom op
  • Functions that are likely to be re-traced

@seanpmorgan seanpmorgan changed the title Remove tf function where unecessary [WIP] Remove tf function where unecessary Jan 2, 2020
@WindQAQ
Copy link
Member

WindQAQ commented Jan 3, 2020

Just a note, have to update the README.md about tf.function description.

# Conflicts:
#	tensorflow_addons/activations/rrelu_test.py
@seanpmorgan seanpmorgan changed the title [WIP] Remove tf function where unecessary Remove tf function where unecessary Jan 5, 2020
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.

Thanks for cleanup! My only concern is that test_unknown_shape for image tests is designed for tf.data.Dataset. If we get rid of tf.function, will it always be compatible for tf.data.Dataset? Moreover, a old thread tensorflow/tensorflow#27811 mentioned that tf.function will be apply automatically for tf.data.Dataset.map for TF2.0.

A trivial test is to use something like

fn = tf.function(dist_ops.euclidean_dist_transform).get_concrete_function(
                 tf.TensorSpec(None, tf.uint8))

to make sure it would be compatible with tf.data.Dataset.

What do you think, Sean?

#330 #332

@seanpmorgan
Copy link
Member Author

Thanks for cleanup! My only concern is that test_unknown_shape for image tests is designed for tf.data.Dataset. If we get rid of tf.function, will it always be compatible for tf.data.Dataset? Moreover, a old thread tensorflow/tensorflow#27811 mentioned that tf.function will be apply automatically for tf.data.Dataset.map for TF2.0.

A trivial test is to use something like

fn = tf.function(dist_ops.euclidean_dist_transform).get_concrete_function(
                 tf.TensorSpec(None, tf.uint8))

to make sure it would be compatible with tf.data.Dataset.

What do you think, Sean?

#330 #332

Thanks good catch.. i've added them back. I was also thinking we should have subpackage test suites similar to keras.layer_test. I'll create a seperate issue for that.

@seanpmorgan seanpmorgan force-pushed the fix-remove-tf-function-custom-ops branch from ad3fae0 to 7cc3d36 Compare January 6, 2020 00:51
@seanpmorgan seanpmorgan changed the title Remove tf function where unecessary [WIP] Remove tf function where unecessary Jan 6, 2020
@seanpmorgan seanpmorgan changed the title [WIP] Remove tf function where unecessary Remove tf function where unecessary Jan 6, 2020
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.

Thanks!

@WindQAQ WindQAQ merged commit 4f88ac5 into tensorflow:master Jan 6, 2020
@seanpmorgan seanpmorgan deleted the fix-remove-tf-function-custom-ops branch January 14, 2020 01:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tf.function bug
3 participants