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

Add visibility parameter to draw_keypoints() #8225

Merged
merged 17 commits into from
Feb 6, 2024

Conversation

bmmtstb
Copy link
Contributor

@bmmtstb bmmtstb commented Jan 20, 2024

Added visibility attribute to draw_keypoints
Added another section to the draw_keypoints example in the gallery
Added tests in test_utils

I'm not entirely sure how to full / properly test the skipped connections, but in visual inspection everything seems fine.

Waiting for your comments @NicolasHug

A few things to note:

  • Within draw_keypoints() num_instances and N gets used to describe the same thing. Possibly double check that.
  • "keypoints", "key_points", or "key points" ? I'm no native so I used the same spelling that was in the documentation by the people before me.
  • I used ~torch.Tensor as type hint for the visibility, not ~torch.BoolTensor.

Added visibility attribute to draw_keypoints
Added example in gallery
Added tests
Copy link

pytorch-bot bot commented Jan 20, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8225

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (27 Unrelated Failures)

As of commit 39d23b7 with merge base 36d0e3e (image):

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@bmmtstb
Copy link
Contributor Author

bmmtstb commented Jan 22, 2024

I diddn't run ufmt and flake on the gallery and tests, my bad. Now everything worked locally.

I'm still wondering about how to display the new prediction matrix. "000.000" looks nice but is not good practice. And if I want the values to align, I can't use spaces.

https://github.com/bmmtstb/vision/blob/dd9b16a8467b689e7e7f55dd57e521669aac8fa1/gallery/others/plot_visualization_utils.py#L473-L491

@NicolasHug NicolasHug changed the title Implementation of #8203 Add visibility parameter to draw_keypoints() Jan 24, 2024
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks a ton @bmmtstb , I made a few comments but this looks great already!

torchvision/utils.py Outdated Show resolved Hide resolved
torchvision/utils.py Outdated Show resolved Hide resolved
torchvision/utils.py Outdated Show resolved Hide resolved
torchvision/utils.py Outdated Show resolved Hide resolved
torchvision/utils.py Outdated Show resolved Hide resolved
gallery/others/plot_visualization_utils.py Outdated Show resolved Hide resolved
gallery/others/plot_visualization_utils.py Show resolved Hide resolved
torchvision/utils.py Outdated Show resolved Hide resolved
test/test_utils.py Show resolved Hide resolved
bmmtstb and others added 5 commits February 5, 2024 00:31
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Added test that compares skeleton creation with visibility
Added test that calls draw_keypoints with visibility of None
Used link draw_keypoints_with_visibility
moved visibility to the back of draw_keypoints call
removed inplace modification of visibility variable
removed redundant comments
@bmmtstb
Copy link
Contributor Author

bmmtstb commented Feb 5, 2024

@NicolasHug I implemented most of your requests.
And for the last open unresolved comment, I referenced the link in the visibility attribute. If that solves it for you, feel free to close it.

Additionally, I could add a test against a dummy image as proposed by you. Sadly I needed to create a new image and couldn't reuse the existing one, see comments above.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thank you so much @bmmtstb , some last comments below but this looks great. I can take care of addressing those comments if you prefer, LMK

torchvision/utils.py Outdated Show resolved Hide resolved
test/test_utils.py Show resolved Hide resolved
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thank you so much @bmmtstb !

@NicolasHug NicolasHug merged commit ae14789 into pytorch:main Feb 6, 2024
21 of 48 checks passed
@bmmtstb bmmtstb deleted the issue-8203 branch February 6, 2024 15:00
facebook-github-bot pushed a commit that referenced this pull request Mar 20, 2024
Reviewed By: vmoens

Differential Revision: D55062800

fbshipit-source-id: fc29a5ea8c5e40b7862a5522bffd73de4c44e080

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Nicolas Hug <nh.nicolas.hug@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants