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 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?
Just find we miss some:
https://github.com/tensorflow/addons/blob/c2faddd818280815bd5e47b81742efdfc90e7aaf/tensorflow_addons/image/distance_transform.py#L34-L36
https://github.com/tensorflow/addons/blob/c2faddd818280815bd5e47b81742efdfc90e7aaf/tensorflow_addons/image/transform_ops.py#L320
https://github.com/tensorflow/addons/blob/c2faddd818280815bd5e47b81742efdfc90e7aaf/tensorflow_addons/image/median_filter_2d.py#L24
Some ops like compose_transforms in transform_ops.py do not have name argument (even when they are in contrib though). Would it be better to add name to them for API compatibility?
rotate op, we should replace str(name) by (name or 'rotate'). 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.@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.
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.