keras-team / keras-cv

Industry-strength Computer Vision workflows with Keras
Other
1k stars 333 forks source link

Adds support in augmentation for keypoints #518

Open atuleu opened 2 years ago

atuleu commented 2 years ago

Short Description

I am interested to use RandAugment for object detection with keypoints. Instead of re-doing everything on my own, I would like to add support for keypoints in the augmentation present in this repo.

Papers

None I know off

Existing Implementations

Already have some PR

Other Information

I would propose to:

atuleu commented 2 years ago

Leaving this work for a few days leaved me with a few questions.

LukeWood commented 2 years ago

Hey @atuleu! Thanks for your enthusiasm and contributions!

Keypoints is definitely in our roadmap, but I want to make sure we get the API right. I'll be sure to spend some time to look over this label type and how other frameworks handle it so we can compare the different approaches.

Are keypoints basically always xy pairs? Are they ever normalized to the image size? These are the questions we need to be sure to answer.

Perhaps @innat, @sayakpaul, or another would know more about the format keypoints tend to come in.

sayakpaul commented 2 years ago

In the past, I have used imgaug for keypoints. In fact, that is what I used in this tutorial: https://keras.io/examples/vision/keypoint_detection/.

You'll notice that the keypoints are normalized w.r.t the spatial resolutions of the input images.

innat commented 2 years ago

@atuleu

I think bounding box augmentation is erroneous for shear and rotation transform. This will lead to larger bounding boxes than the bounding box covering the sheared/rotated object. Shouldn't the library show some kind of warning if approximative bounding boxes augmentation are made ?

Bounding boxes should be clipped in that cases and same goes to keypoint type as well. Isn't it take in account by already in shear transformation? Mentioning @pranavjadhav001 to comment on this as he's working on it now.

innat commented 2 years ago

@LukeWood

I used albumentation for keypoint augmentation and if I remember correctly I didn't have to normalize keypoint to the image size. The keypoint are usually comes in xy pairs. You can go through the following albumentaiton blogs regarding their approaches and API.

LukeWood commented 2 years ago

so maybe we should mirror the bounding box API, but instead of offering a ton of formats only two: xy, rel_xy. Some augmentations (i.e. rotation) will behave a bit differently depending on format, so we cant just assume one.

Thoughts?

sayakpaul commented 2 years ago

Nice one! Reducing the supported formats for this might actually be a better UX.

atuleu commented 2 years ago

@atuleu

I think bounding box augmentation is erroneous for shear and rotation transform. This will lead to larger bounding boxes than the bounding box covering the sheared/rotated object. Shouldn't the library show some kind of warning if approximative bounding boxes augmentation are made ?

Bounding boxes should be clipped in that cases and same goes to keypoint type as well. Isn't it take in account by already in shear transformation? Mentioning @pranavjadhav001 to comment on this as he's working on it now.

@innat

So there was no augment_bounding_box() for RandomShear ( I added it here aa4a178a60bbdccfb9c4881409a652363c51e45b ). There was last week some augment_bounding_box() for RandomRotation , but no clipping.

@LukeWood I think we should go for a reduced amount of format of 'xy' and 'rel_xy'. Then if more format are needed, they could always be added afterwards. Should I go forward and add this to #519 ?

atuleu commented 2 years ago

I won't be able to work on this for a few days, but would happily add keypoints format support to #519 in the end of next week

atuleu commented 2 years ago

So in #519, I Added the following:

atuleu commented 2 years ago

Following the discussion in #519, here are the tasks to perform in single PR from atuleu/keras-cv@dev/keypoints:

LukeWood commented 2 years ago

Thank you @atuleu for breaking the problem down like this. I appreciate your contributions greatly. Pleasure working together, I'll review these as they come in!

pranavjadhav001 commented 2 years ago

@atuleu

I think bounding box augmentation is erroneous for shear and rotation transform. This will lead to larger bounding boxes than the bounding box covering the sheared/rotated object. Shouldn't the library show some kind of warning if approximative bounding boxes augmentation are made ?

Bounding boxes should be clipped in that cases and same goes to keypoint type as well. Isn't it take in account by already in shear transformation? Mentioning @pranavjadhav001 to comment on this as he's working on it now.

For random shear bbox augmentation , we are clipping bboxes to 0- image width/height.

atuleu commented 2 years ago

Request For Comment @LukeWood, @inat, @sayakpaul

I think I am hitting an initial design choice issue at the moment, and I would need some input to help me choose the best options.

The issue comes from the handling of keypoints that may become invalid after an augmentation. My initial thought has suggested by #585 was to simply return in augment_keypoints a ragged tensor of the valid keypoints. It works well with single images, but if you throw in some vectorization, or sequence of transformation (by example in a RandomAugmentationPipeline), well it does not work well. And from reading other issues, RaggedTensor won´t work well with XLA.

Then I would propose again a new design change from what I proposed in #586 :

divyashreepathihalli commented 8 months ago

Hi @atuleu do you have a PR for adding this support?

atuleu commented 8 months ago

Hello,

There was more than a year ago one huge PR to add suport on keypoint. I was asked to make multiple PR, but the very first one, that was held back by the main maintainers. I tried several to update this PR to upstream change but gave up as it seems the repo's maintainer did not want to merge it.

My day to day work is not relying on keras-cv anymore (very much for these reason). I have therefore no incentive to make this PR happen, with the very likeliness it won't get merged again.

Bests,

On Mon, 5 Feb 2024 at 20:12, Divyashree Sreepathihalli < @.***> wrote:

Hi @atuleu https://github.com/atuleu do you have a PR for adding this support?

— Reply to this email directly, view it on GitHub https://github.com/keras-team/keras-cv/issues/518#issuecomment-1927859978, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACCZ3ZH33SDUVR2OCZIZQTYSEVLBAVCNFSM5ZVU7VSKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOJSG44DKOJZG44A . You are receiving this because you were mentioned.Message ID: @.***>

divyashreepathihalli commented 8 months ago

@atuleu I am sorry to hear that. I do not have much context on this, but I do think this is valuable. I will leave the issue open to track it.