Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 🚀 initial keypoint support for transformers added #1553

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

onuralpszr
Copy link
Collaborator

@onuralpszr onuralpszr commented Sep 27, 2024

Description

Recent release of transformers introduce keypoint support and initial support for keypoint added.

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Docs

  • Docs updated? What were the changes:

Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
@onuralpszr onuralpszr self-assigned this Sep 27, 2024
@onuralpszr
Copy link
Collaborator Author

onuralpszr commented Sep 27, 2024

@merveenoyan 👋 hello I notice current latest and dev doesn't have "post_process_keypoint_detection" function and I was able to run via only this pull request branch "huggingface/transformers#33200" Today I read about "https://huggingface.co/docs/transformers/tasks/keypoint_detection" you posted in social and I was unable run keypoint but I was able to do via using install this repo/branch, I think it is forgot to merge ?

!pip install git+https://github.com/sbucaille/transformers@superpoint_fix -q

Error output when I ran from "main"

    outputs = processor.post_process_keypoint_detection(outputs, image_sizes)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'SuperPointImageProcessor' object has no attribute 'post_process_keypoint_detection'

Here is my initial collab link for working version

https://colab.research.google.com/drive/1gxpE9iDR7gXl2VwwScB5W8Yw-e_xUo3i?usp=sharing

@onuralpszr onuralpszr changed the title feat: 🚀 initial keypoint support for transformers added WIP - feat: 🚀 initial keypoint support for transformers added Sep 27, 2024
@onuralpszr
Copy link
Collaborator Author

@LinasKo missing function basically converting normalize values to w,h values to position correctly should I finish based on that in current release and finish the PR ?

@LinasKo
Copy link
Collaborator

LinasKo commented Oct 1, 2024

@onuralpszr, I need more details.

  1. Missing function where? Our repo or theirs?
  2. "should I finish based on that in current release and finish the PR ?" - Based on what? Based on the function being missing? I'd like more clarity here 😉

@onuralpszr
Copy link
Collaborator Author

@onuralpszr, I need more details.

  1. Missing function where? Our repo or theirs?
  2. "should I finish based on that in current release and finish the PR ?" - Based on what? Based on the function being missing? I'd like more clarity here 😉

Sorry, I was little bit hasty I guess and for answering your questions

1 - Missing function is theirs and it is post process function for converting normalize xy values to pixel based on xy values (post_process_keypoint_detection is missing)
2 - Currently latest version of transfomers has SuperPointImageProcessor but, doesn't have SuperPointImageProcessor.post_process_keypoint_detection() so I will finish PR based on latest version of transformers to and I will write the helper function to convert image size correction or I had to assume it does use function and leave as it is. You can check the PR link I posted above to see it.

@onuralpszr
Copy link
Collaborator Author

@LinasKo I see approve so I am going to move forward based on this PR ("huggingface/transformers#33200" ) I am going to finish this PR

Copy link
Collaborator

@LinasKo LinasKo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a few small comments. Let's wait until they merge the change, test it in a Colab, make sure docs are available via mkdocs serve and then merge it!

supervision/keypoint/core.py Outdated Show resolved Hide resolved
supervision/keypoint/core.py Outdated Show resolved Hide resolved
return cls(
xy=np.array(keypoints_list),
confidence=np.array(scores_list),
class_id=None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to explicitly set it to None?

@onuralpszr onuralpszr changed the title WIP - feat: 🚀 initial keypoint support for transformers added feat: 🚀 initial keypoint support for transformers added Nov 1, 2024
…and class_id removed

Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
Signed-off-by: Onuralp SEZER <thunderbirdtr@gmail.com>
@LinasKo
Copy link
Collaborator

LinasKo commented Nov 2, 2024

Hey @onuralpszr ,

How's this one going? Is it ready for review, or is there some work left to do?

@onuralpszr
Copy link
Collaborator Author

Hey @onuralpszr ,

How's this one going? Is it ready for review, or is there some work left to do?

I have small problem when I detect multiple images at once keypoint list doesn't come in same sizes. Point lists are different

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants