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.

Sign up for GitHub

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

Jump to bottom

Properly handle maskrcnn and keypoints w.r.t. V2 in detection references #7742

Merged
merged 8 commits into from
Jul 27, 2023
Merged

Properly handle maskrcnn and keypoints w.r.t. V2 in detection references #7742

merged 8 commits into from
Jul 27, 2023

Conversation

NicolasHug
Copy link
Member

  • a bunch of cleanups and better error messages

@pytorch-bot
Copy link

pytorch-bot bot commented Jul 13, 2023 •
edited
Loading

🔗 Helpful Links

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

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

❌ 21 New Failures

As of commit 511f323:

NEW FAILURES - The following jobs have failed:

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

@NicolasHug NicolasHug requested a review from pmeier July 13, 2023 15:56
@NicolasHug NicolasHug requested a review from vfdev-5 July 13, 2023 15:56
pmeier
pmeier reviewed Jul 24, 2023
View reviewed changes
references/detection/train.py Outdated Show resolved Hide resolved
references/detection/train.py Outdated Show resolved Hide resolved
references/detection/train.py Outdated Show resolved Hide resolved
references/detection/coco_utils.py Outdated Show resolved Hide resolved
references/detection/coco_utils.py Show resolved Hide resolved
NicolasHug
NicolasHug commented Jul 24, 2023
View reviewed changes
references/detection/train.py

ds = ds_fn(p, image_set=image_set, transforms=get_transform(is_train, args), use_v2=args.use_v2)
num_classes, mode = {"coco": (91, "instances"), "coco_kp": (2, "person_keypoints")}[args.dataset]
with_masks = "mask" in args.model
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should remove the get_masks parameter of get_coco() and just make it part of the mode parameter, i.e. allow "instances", "instances_masks", and "keypoints".

ooooor just completely change the parametrization of get_coco() to something more friendly.

In addition to that, the way we magically specify keypoints or masks (through dataset name or through model name) is pretty terrible right now.

I feel like we should address that for good, but I'd prefer doing it in another PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree that it would be better that args.dataset could parametrize data type instead of the model name.
args.dataset can be "coco, "coco_kp" and "coco_masks", for example.

@pytorch pytorch deleted a comment from jp1010620676 Jul 24, 2023
vfdev-5
vfdev-5 approved these changes Jul 27, 2023
View reviewed changes
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Looks good to me

pmeier
pmeier approved these changes Jul 27, 2023
View reviewed changes
Copy link
Collaborator

@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!

@NicolasHug NicolasHug merged commit 8233c9c into pytorch:main Jul 27, 2023
39 of 60 checks passed
@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

facebook-github-bot pushed a commit that referenced this pull request Aug 25, 2023
@NicolasHug @facebook-github-bot
[fbsync] Properly handle maskrcnn and keypoints w.r.t. V2 in detectio…
08898a0 
…n references (#7742)

Summary: Co-authored-by: Philip Meier <github.pmeier@posteo.de>

Reviewed By: matteobettini

Differential Revision: D48642313

fbshipit-source-id: 2bf2bdbaef94dc7851646581bb922fc69fc42e77
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vfdev-5 vfdev-5 vfdev-5 approved these changes

@pmeier pmeier pmeier approved these changes

Assignees
No one assigned
Labels
cla signed enhancement module: reference scripts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NicolasHug @vfdev-5 @pmeier @facebook-github-bot