Skip to content

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Apr 25, 2022

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

Thanks @NicolasHug, I've added a few comments below to get your thoughts. Happy to chat more.


generate_classification_table()
generate_weights_table(module=M, table_name="classification", metrics_names=["Acc@1", "Acc@5"])
generate_weights_table(module=M.detection, table_name="detection", metrics_names=["box_map"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is going to put the KeypointRCNN model along with the rest of the models. Unfortunately the specific model was trained with different data from COCO (keypoints), so its focus and validation numbers are the same as for other models.

Have a look on the current documentation here to see how the models are grouped and how extra accuracy metrics are being shown. We don't have to do exactly the same thing but good to get some ideas.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it OK with you if I create an issue to special-case KeypointRCNN? This way I'll still be able to ask for external contributors while I'm thinking of a decent solution for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that would be fine, let's do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I opened #5882 to keep track of this

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

Approving since on principle it looks good. Just one non-blocking nit below:

weights = [w for weight_enum in weight_enums for w in weight_enum]

column_names = ("**Weight**", "**Acc@1**", "**Acc@5**", "**Params**", "**Recipe**")
def clean_name(name):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Might be cleaner if you pass metrics_names as Tuple[str, str] where one part of the tuple is the key and the second the doc-friendly name of it.


generate_classification_table()
generate_weights_table(module=M, table_name="classification", metrics_names=["Acc@1", "Acc@5"])
generate_weights_table(module=M.detection, table_name="detection", metrics_names=["box_map"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think that would be fine, let's do this.

@NicolasHug NicolasHug merged commit d425f00 into pytorch:main Apr 26, 2022
@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 May 6, 2022
Summary:
* Start doc revamp for detection models

* Minor cleanup

* Use list of tuples for metrics

Reviewed By: jdsgomes, NicolasHug

Differential Revision: D36095699

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