Skip to content

Conversation

@NicolasHug
Copy link
Member

This PR adds support for tuples in the default heuristic of SanitizeBoundingBoxes.

This is needed because a lot of our built-in datasets return (img, dict_of_stuff) instead of just dict, which is what the current heuristic expects. This addresses a current UX painpoint #7302 (comment)

Comment on lines +2026 to +2027
if sample_type is tuple:
sample = (None, sample, "whatever_again")
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this show a bug in the implementation? Shouldn't we require the tuple to only have two elements?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can, I just went for this because it felt like an artificial restriction. I.e. I don't see more things going wrong by allowing tuples with 3+ entries. And not raising an error actually makes the code simpler. But I don't have a strong opinion

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Maybe put a note of that in the comment of _get_dict_or_second_tuple_entry. So far it only uses a two-tuple as example.

Copy link
Contributor

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks Nicolas, LGTM if CI is green.

Comment on lines +2026 to +2027
if sample_type is tuple:
sample = (None, sample, "whatever_again")
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Maybe put a note of that in the comment of _get_dict_or_second_tuple_entry. So far it only uses a two-tuple as example.

@NicolasHug NicolasHug merged commit ed48bb1 into pytorch:main Feb 23, 2023
@github-actions
Copy link

Hey @NicolasHug!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

NicolasHug added a commit to NicolasHug/vision that referenced this pull request Feb 24, 2023
facebook-github-bot pushed a commit that referenced this pull request Mar 29, 2023
… tuples (#7304)

Summary: Co-authored-by: Philip Meier <[email protected]>

Reviewed By: vmoens

Differential Revision: D44416594

fbshipit-source-id: dc22119387695f5d8583f0745b2c80260a9d8b87
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