Addons: ImageProjectiveTransform should be named ImageProjectiveTransformV2

Created on 12 Mar 2019  路  8Comments  路  Source: tensorflow/addons

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.
bug good first issue help wanted image

Most helpful comment

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.

All 8 comments

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.

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?

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.

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

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

Two match:

  • AdjustHsvInYiq [[contrib](https://github.com/tensorflow/tensorflow/blob/master/tensorflow/contrib/image/ops/distort_image_ops.cc) | [addon](https://github.com/tensorflow/addons/blob/master/tensorflow_addons/custom_ops/image/cc/ops/distort_image_ops.cc)]
  • SkipGramGenerateCandidates [[contrib](https://github.com/tensorflow/tensorflow/blob/master/tensorflow/contrib/text/ops/skip_gram_ops.cc) | [addon](https://github.com/tensorflow/addons/blob/master/tensorflow_addons/custom_ops/text/cc/ops/skip_gram_ops.cc)]

As noted, one has a name mismatch:

  • ImageProjectiveTransformV2 [contrib] became ImageProjectiveTransform [[addon](https://github.com/tensorflow/addons/blob/master/tensorflow_addons/custom_ops/image/cc/ops/image_ops.cc#L95)]

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

I think for now, yes -- that'll make sure that people can use the existing
SavedModels, etc.

For the future, we need to figure out proper namespacing.

cc @tomerk

Was this page helpful?
0 / 5 - 0 ratings

Related issues

maziyarpanahi picture maziyarpanahi  路  3Comments

gabrieldemarmiesse picture gabrieldemarmiesse  路  3Comments

seanpmorgan picture seanpmorgan  路  3Comments

shun-lin picture shun-lin  路  4Comments

seanpmorgan picture seanpmorgan  路  3Comments