Addons: Unify styles of custom operations

Created on 10 Apr 2019  路  7Comments  路  Source: tensorflow/addons

Currently, custom operations in addons have no unified styles. For example
the name argument in
https://github.com/tensorflow/addons/blob/463112e6e7c8d3623d3d97e6b1f365d1c403f2c4/tensorflow_addons/image/dense_image_warp.py#L161
and
https://github.com/tensorflow/addons/blob/463112e6e7c8d3623d3d97e6b1f365d1c403f2c4/tensorflow_addons/image/transform_ops.py#L38-L42
are not consistent.

As per coding style for python operation in Tensorflow, the last argument should be name with a default value of None. Though not all operations in core Tensorflow conform to this (see tf.image source code), I'd suppose we should unify the API/style of python operations in addons (like what we do for losses and layers). Maybe we can try to follow the link above first.

good first issue style

Most helpful comment

The name arg problem is annoying.

We're not quite ready to remove the name args. At this point, they're exclusively used for visualization in TensorBoard. We have some ideas about visualizing summarized stack traces instead of manually set names, and once that works well, we can think about soft deprecating all name args.

For now, I'd keep the name args.

All 7 comments

Good catch, I think None is better for default value, because it's easy to maintain API compatibility.

Got it, I'll create a PR for image ops first.

I think this is complete? Are there any other ops that need modifying?

  1. For rotate op, we should replace str(name) by (name or 'rotate').
  2. There exists an known annoying issue about name: if we want to add new attribute, we have to append it after name for ensuring backward compatibility. And all new attributes are forced to be optional. But I agree to conform to tensorflow convention: add name for all public API in image module.
    cc @martinwicke Hi, Martin, do you have any good idea?

@seanpmorgan Sean, what do you think?

The name arg problem is annoying.

We're not quite ready to remove the name args. At this point, they're exclusively used for visualization in TensorBoard. We have some ideas about visualizing summarized stack traces instead of manually set names, and once that works well, we can think about soft deprecating all name args.

For now, I'd keep the name args.

Thank @martinwicke for the input.

@WindQAQ Tzu-Wei, let's add name argument for all public operators.

Was this page helpful?
0 / 5 - 0 ratings