🚨🚨🚨 [SuperPoint] Fix keypoint coordinate output and add post processing by sbucaille · Pull Request #33200 · huggingface/transformers

sbucaille · 2024-08-29T15:06:24Z

What does this PR do?

There is a bug highlighted by @merveenoyan where SuperPoint returns absolute keypoint coordinates at a wrong scale.
For example, an image of size (1000, 1000) will first be resized into (640, 480) by the image processor but SuperPoint will output keypoint coordinates between 0 and 640 for x and 0 and 480 for y.
This PR fixes this bug by returning keypoints in relative coordinates out of the SuperPoint forward method.
An additional post_process_keypoint_detection is added to the SuperPointImageProcessor to cast the keypoint coordinates into the original image sizes.
I also added an "unwrapping" logic where the output will be a list of keypoints, scores, descriptors for each image so that the user don't have to deal with the current mask mechanism. This is conditionned by a boolean argument in the signature.

I have not finished all the details but most of the implementation is there and I will add tests tomorrow but at least the PR is open as I promised Merve ☺️

Fixes #33825

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline, Pull Request section?
Was this discussed/approved via a Github issue or the forum? Please add a link
to it if that's the case.
-> On Slack
Did you make sure to update the documentation with your changes? Here are the
documentation guidelines, and
here are tips on formatting docstrings.
Did you write any new necessary tests?

Who can review?

@amyeroberts @NielsRogge @merveenoyan

amyeroberts

Thanks for fixing and adding this method!

Overall looks good - just a few things to add / update

src/transformers/models/superpoint/image_processing_superpoint.py

amyeroberts · 2024-08-29T15:22:49Z

src/transformers/models/superpoint/modeling_superpoint.py

+        # Convert to relative coordinates
+        keypoints = keypoints / torch.tensor([width, height], device=keypoints.device)


Technically a breaking change - but as this is more of a fix, I think it's OK

…ageProcessor

…and switched from opencv to matplotlib

…in the example

…st_tf" This reverts commit 39b32a2.

docs/source/en/model_doc/superpoint.md

src/transformers/models/superpoint/image_processing_superpoint.py

Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>

…e choice in post process method

docs/source/en/model_doc/superpoint.md

amyeroberts

Thanks for iterating!

Mostly small comments. Main one is about the indexing on image_sizes in the post-processing method

src/transformers/models/superpoint/image_processing_superpoint.py

tests/models/superpoint/test_image_processing_superpoint.py

docs/source/en/model_doc/superpoint.md

sbucaille · 2024-09-05T19:50:23Z

@amyeroberts comments addressed, tests are passing, let me know if I can do a final rebase for the merge

HuggingFaceDocBuilderDev · 2024-09-06T09:26:44Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Co-authored-by: Pavel Iakubovskii <qubvel@gmail.com>

… statement and changed tests results to reflect changes to SuperPoint from absolute keypoints coordinates to relative

…sing

sbucaille · 2024-10-02T20:33:27Z

@qubvel thanks for the review, I've addressed your comments. I initially forgot to change the tests to reflect the changes in terms of SuperPoint's outputs, so this is also fixed. Let me know if there is anything else to do

qubvel

Thanks quick iteration, a few more comments

src/transformers/models/superpoint/image_processing_superpoint.py

src/transformers/models/superpoint/modeling_superpoint.py

qubvel · 2024-10-03T10:18:47Z

tests/models/superpoint/test_modeling_superpoint.py

        inputs = preprocessor(images=images, return_tensors="pt").to(torch_device)
        with torch.no_grad():
            outputs = model(**inputs)
-        expected_number_keypoints_image0 = 567


Wondering why the number of keypoints changed :)

tests/models/superpoint/test_modeling_superpoint.py

…ocess_keypoint_detection

…oder class name and added relative (x, y) coordinates information to its method

qubvel

Thank you for the fixes! Test diff looks fine now 👍

src/transformers/models/superpoint/modeling_superpoint.py

tests/models/superpoint/test_modeling_superpoint.py

src/transformers/models/superpoint/modeling_superpoint.py

qubvel

Thanks, looks good to me now!

cc @ArthurZucker for final review

docs/source/en/model_doc/superpoint.md

Co-authored-by: Pavel Iakubovskii <qubvel@gmail.com>

ArthurZucker

Feel free to merge if ready for you @qubvel 🤗

onuralpszr · 2024-10-29T01:48:21Z

Hello, I was following this PR and curious that is it going to merge or any change still require ?

sbucaille · 2024-10-29T08:18:51Z

Hi @onuralpszr ,
Thanks for asking, there is no change required on my side !

qubvel · 2024-10-29T09:36:51Z

@sbucaille thanks a lot for your contribution!

…ng (huggingface#33200) * feat: Added int conversion and unwrapping * test: added tests for post_process_keypoint_detection of SuperPointImageProcessor * docs: changed docs to include post_process_keypoint_detection method and switched from opencv to matplotlib * test: changed test to not depend on SuperPointModel forward * test: added missing require_torch decorator * docs: changed pyplot parameters for the keypoints to be more visible in the example * tests: changed import torch location to make test_flax and test_tf * Revert "tests: changed import torch location to make test_flax and test_tf" This reverts commit 39b32a2. * tests: fixed import * chore: applied suggestions from code review Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com> * tests: fixed import * tests: fixed import (bis) * tests: fixed import (ter) * feat: added choice of type for target_size and changed tests accordingly * docs: updated code snippet to reflect the addition of target size type choice in post process method * tests: fixed imports (...) * tests: fixed imports (...) * style: formatting file * docs: fixed typo from image[0] to image.size[0] * docs: added output image and fixed some tests * Update docs/source/en/model_doc/superpoint.md Co-authored-by: Pavel Iakubovskii <qubvel@gmail.com> * fix: included SuperPointKeypointDescriptionOutput in TYPE_CHECKING if statement and changed tests results to reflect changes to SuperPoint from absolute keypoints coordinates to relative * docs: changed SuperPoint's docs to print output instead of just accessing * style: applied make style * docs: added missing output type and precision in docstring of post_process_keypoint_detection * perf: deleted loop to perform keypoint conversion in one statement * fix: moved keypoint conversion at the end of model forward * docs: changed SuperPointInterestPointDecoder to SuperPointKeypointDecoder class name and added relative (x, y) coordinates information to its method * fix: changed type hint * refactor: removed unnecessary brackets * revert: SuperPointKeypointDecoder to SuperPointInterestPointDecoder * Update docs/source/en/model_doc/superpoint.md Co-authored-by: Pavel Iakubovskii <qubvel@gmail.com> --------- Co-authored-by: Steven Bucaille <steven.bucaille@buawei.com> Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com> Co-authored-by: Pavel Iakubovskii <qubvel@gmail.com>

feat: Added int conversion and unwrapping

17d50e8

amyeroberts reviewed Aug 29, 2024

View reviewed changes

sbucaille added 8 commits August 30, 2024 11:46

test: added tests for post_process_keypoint_detection of SuperPointIm…

285c465

…ageProcessor

docs: changed docs to include post_process_keypoint_detection method …

2efe61b

…and switched from opencv to matplotlib

test: changed test to not depend on SuperPointModel forward

a77b870

test: added missing require_torch decorator

2ab79cd

docs: changed pyplot parameters for the keypoints to be more visible …

419ae5d

…in the example

tests: changed import torch location to make test_flax and test_tf

39b32a2

Revert "tests: changed import torch location to make test_flax and te…

144e09a

…st_tf" This reverts commit 39b32a2.

tests: fixed import

21dbdfc

NielsRogge reviewed Sep 1, 2024

View reviewed changes

docs/source/en/model_doc/superpoint.md Outdated Show resolved Hide resolved

NielsRogge reviewed Sep 1, 2024

View reviewed changes

docs/source/en/model_doc/superpoint.md Show resolved Hide resolved

merveenoyan reviewed Sep 1, 2024

View reviewed changes

src/transformers/models/superpoint/image_processing_superpoint.py Outdated Show resolved Hide resolved

sbucaille and others added 9 commits September 1, 2024 14:59

chore: applied suggestions from code review

389b154

Co-authored-by: NielsRogge <48327001+NielsRogge@users.noreply.github.com>

tests: fixed import

b7d672e

tests: fixed import (bis)

f5d7311

tests: fixed import (ter)

d89d385

feat: added choice of type for target_size and changed tests accordingly

f9e1141

docs: updated code snippet to reflect the addition of target size typ…

32a2e96

…e choice in post process method

tests: fixed imports (...)

560194e

tests: fixed imports (...)

2d28aba

style: formatting file

bd23baa

NielsRogge requested a review from amyeroberts September 2, 2024 08:43

merveenoyan reviewed Sep 2, 2024

View reviewed changes

docs/source/en/model_doc/superpoint.md Outdated Show resolved Hide resolved

docs: fixed typo from image[0] to image.size[0]

5bb0baf

amyeroberts reviewed Sep 5, 2024

View reviewed changes

docs: added output image and fixed some tests

ed28314

onuralpszr mentioned this pull request Sep 27, 2024

feat: 🚀 initial keypoint support for transformers added roboflow/supervision#1553

Closed

3 tasks

sbucaille and others added 4 commits October 2, 2024 21:53

Update docs/source/en/model_doc/superpoint.md

192448d

Co-authored-by: Pavel Iakubovskii <qubvel@gmail.com>

fix: included SuperPointKeypointDescriptionOutput in TYPE_CHECKING if…

e89af7f

… statement and changed tests results to reflect changes to SuperPoint from absolute keypoints coordinates to relative

docs: changed SuperPoint's docs to print output instead of just acces…

4e77a4f

…sing

style: applied make style

e9b642a

qubvel added Vision bug labels Oct 3, 2024

qubvel changed the title ~~[SuperPoint] Fix keypoint coordinate output and add post processing~~ 🚨🚨🚨 [SuperPoint] Fix keypoint coordinate output and add post processing Oct 3, 2024

qubvel reviewed Oct 3, 2024

View reviewed changes

sbucaille added 5 commits October 3, 2024 23:12

docs: added missing output type and precision in docstring of post_pr…

e085861

…ocess_keypoint_detection

perf: deleted loop to perform keypoint conversion in one statement

9127545

fix: moved keypoint conversion at the end of model forward

1ffa465

docs: changed SuperPointInterestPointDecoder to SuperPointKeypointDec…

b0d25a3

…oder class name and added relative (x, y) coordinates information to its method

fix: changed type hint

1fb5705

qubvel reviewed Oct 4, 2024

View reviewed changes

src/transformers/models/superpoint/modeling_superpoint.py Outdated Show resolved Hide resolved

tests/models/superpoint/test_modeling_superpoint.py Show resolved Hide resolved

src/transformers/models/superpoint/modeling_superpoint.py Outdated Show resolved Hide resolved

sbucaille added 2 commits October 4, 2024 13:44

refactor: removed unnecessary brackets

13cb7e5

revert: SuperPointKeypointDecoder to SuperPointInterestPointDecoder

eb6a5aa

qubvel self-requested a review October 4, 2024 17:30

qubvel reviewed Oct 4, 2024

View reviewed changes

docs/source/en/model_doc/superpoint.md Outdated Show resolved Hide resolved

Update docs/source/en/model_doc/superpoint.md

4c34d75

Co-authored-by: Pavel Iakubovskii <qubvel@gmail.com>

ArthurZucker self-requested a review October 5, 2024 14:33

sbucaille mentioned this pull request Oct 8, 2024

Add SuperGlue model #29886

Merged

ArthurZucker approved these changes Oct 15, 2024

View reviewed changes

qubvel merged commit a183519 into huggingface:main Oct 29, 2024

sbucaille deleted the superpoint_fix branch October 29, 2024 09:42

		# Convert to relative coordinates
		keypoints = keypoints / torch.tensor([width, height], device=keypoints.device)

Conversation

sbucaille commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Before submitting

Who can review?

Uh oh!

amyeroberts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amyeroberts Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amyeroberts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbucaille commented Sep 5, 2024

Uh oh!

HuggingFaceDocBuilderDev commented Sep 6, 2024

Uh oh!

sbucaille commented Oct 2, 2024

Uh oh!

qubvel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qubvel Oct 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qubvel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qubvel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ArthurZucker left a comment

Choose a reason for hiding this comment

Uh oh!

onuralpszr commented Oct 29, 2024

Uh oh!

sbucaille commented Oct 29, 2024

Uh oh!

qubvel commented Oct 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sbucaille commented Aug 29, 2024 •

edited

Loading