search

keras-team / keras-cv

Industry-strength Computer Vision workflows with Keras
Other
1.01k stars 330 forks source link

Reduce to shorter namespace #2009

Closed pure-rgb closed 1 year ago

pure-rgb commented 1 year ago

Hello keras-cv team,

I followed some of the code examples written in keras-cv and some

While working with the library, I noticed that some namespaces for key methods can be quite long. For example, keras_cv.bounding_box.convert_format. I found that the length of the namespace can make the code slightly more cumbersome to write and read. And also it looks like this should be under the keras_cv.utils API. There are also some other places I found to conflict with another library (i.e. torch).

It feels natural to seek some of these APIs from the keras.utils API. Also, the bounding box can be renamed to bbox or just box for the sake of brevity. 

keras_cv.bounding_box.convert_format(...)
keras_cv.bounding_box.clip_to_image(...)
keras_cv.bounding_box.compute_iou(...)

I understand that the length of the namespace can sometimes reflect the structure of the library. However, if there's a way to shorten these paths without losing the underlying logic, I believe it would be a helpful improvement.

ianstenbit commented 1 year ago

I see where you're coming from with this -- and I do think that we should consider some changes to namespacing as long as we're consistent with the Keras API. That said, the Keras API does not include abbreviations -- we like to spell out full words because it makes for a more readable API, and the extra letters switching from convert to cvt doesn't really buy us anything.

That said -- now that we're using keras_cv_export, we can export the same symbol as multiple paths! So for example we could say keras_cv.bounding_box.convert_format could also be exported as keras_cv.utils.convert_box_format.

WRT the bounding_box name in particular -- I am pretty indifferent about bounding_box vs box, and I'm personally happy with exporting as either both or switching to box. I'm not aware of why bounding_box was chosen initially so I would like to understand that before making the change.

If there are individual example components you'd like to propose a new path for, feel free to open a PR updating the keras_cv_export for that symbol.

pure-rgb commented 1 year ago

we like to spell out full words because it makes for a more readable API,

Not really, but it depends. It would be better for keras to be more flexible. It can take an example from another well-established framework, i.e. torch, transformers. TBH, one of my colleagues mentioned disliking tf/keras because of the long naming convention.

extra letters switching from convert to cvt doesn't really buy us anything.

The cvt here was an example, doesn't have to be this. The main point here is the same. FYI, the cvt here was inspired by opencv. Or, if you look at torch for references, torchvision.ops.box_convert

That said -- now that we're using keras_cv_export, we can export the same symbol as multiple paths! So for example we could say keras_cv.bounding_box.convert_format could also be exported as keras_cv.utils.convert_box_format

Didn't have any idea about this. But looking at the above example, I think keras should avoid multiple namespaces to achieve the same functionality. The tensorflow has a bad reputation to have multiple namespaces to achieve the same functionality. Please check below, the following APIs, collected from this source.

tf.nn.conv2d tf.keras.layers.Conv2D tf.compat.v1.layers.Conv2D tf.compat.v1.layers.conv2d tf.layers.Conv2D tf.raw_ops.Conv2D tf.contrib.slim.conv2d

That is all.