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

ImageProjectiveTransform should be named ImageProjectiveTransformV2 #86

Closed
andresusanopinto opened this issue Mar 12, 2019 · 8 comments
Closed
Labels
bug Something isn't working good first issue help wanted Needs help as a contribution image

Comments

@andresusanopinto
Copy link

andresusanopinto commented Mar 12, 2019

Describe the bug

  • It looks like ImageProjectiveTransform is based on ImageProjectiveTransformV2 present in tf.contrib in TF-1.x.

Describe the expected behavior

  • Op names should not colide between 1.x and 2.x environments or saved models won't be shareable across them.
@andresusanopinto
Copy link
Author

Links to ops:

Looking at code it really seems that the op "ImageProjectiveTransformV2" from tf.contrib.image was renamed to "ImageProjectiveTransform" (which was a name already taken by other op). I guess this was un-intentional or in an attempt to cleanup the names, but in reality it can break moving models across TF-1.x and TF-2.x.

@seanpmorgan
Copy link
Member

Thanks @andresusanopinto. I think you are correct that this is a problem even though we don't support 1.x because of saved models.

@karmel @martinwicke Should we maintain all V2 op naming just to be safe?

@seanpmorgan seanpmorgan added bug Something isn't working image labels Mar 12, 2019
@karmel
Copy link
Contributor

karmel commented Mar 12, 2019

Yes-- we continue to support v1 graphdef/ops in v2, so maintaining separate namespaces will help avoid collisions. Eventually, we will be able to prefix op names with appropriate namespaces (ie, tensorflow_addons:OP_NAME), but we don't do that yet.

@seanpmorgan seanpmorgan added help wanted Needs help as a contribution good first issue labels Mar 12, 2019
@seanpmorgan
Copy link
Member

Thanks! Adding help wanted, and this bug fix would require reviewing all Ops that we've moved over.

@jharmsen
Copy link
Contributor

Looks like there are only three ops in addon so far.

Two match:

As noted, one has a name mismatch:

  • ImageProjectiveTransformV2 [contrib] became ImageProjectiveTransform [addon]

@jharmsen
Copy link
Contributor

Going forward is the right thing to rename addon ImageProjectiveTransform to ImageProjectiveTransformV2?

@martinwicke
Copy link
Member

martinwicke commented Mar 13, 2019 via email

@karmel
Copy link
Contributor

karmel commented May 15, 2019

cc @tomerk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue help wanted Needs help as a contribution image
Projects
None yet
Development

No branches or pull requests

5 participants