Document Keypoint RCNN separately

[NicolasHug]

Closes #5882

This PR documents Keypoint RCNN separately, in its own section and with its own table.

[NicolasHug]

Not a die-hard fan of this, but we have to special-case this generation function (or its inputs) in some way. Happy to consider other suggestions.

[jdsgomes]

The other alternative makes the code simpler here but moves complexity to the caller, i.e., they could provide a validation pattern as regex.

def generate_weights_table(module, table_name, metrics, include_pattern=".*"):
    weight_enums = [getattr(module, name) for name in dir(module) if name.endswith("_Weights")]
    weights = [w for weight_enum in weight_enums for w in weight_enum if re.match(include_pattern, w)]
    ...

regex would include both include and exclude patterns

[NicolasHug]

I thought about using regex, but I could not find a include_pattern where

re.match(include_pattern, w)

would be Truthy for weights that do not contain "Keypoint". I'm sure it's possible, but the complexity of the resulting regex might outweight the complexity of the current code. Any pointer @jdsgomes ?

[jdsgomes]

I think this should do the trick:

In [59]:  re.match("^((?!Keypoint).)*$", "no_keypoint_in_name")
Out[59]: <re.Match object; span=(0, 19), match='no_keypoint_in_name'>

vs

In [58]: re.match("^((?!Keypoint).)*$", "xxxKeypoint")
Out[58]: None

[NicolasHug]

Thanks @jdsgomes , that seems to work indeed. I'm a bit on the fence with this. In general, I try to avoid regex like the plague. This one typically doesn't read easily to me and would require some extra comment IMHO.

I'll yield to whichever you prefer. Do you have a preference here?

[jdsgomes]

since we are including/excluding simple patterns I would agree that regex here might just complicate things and just save a few lines of code, so I would say to leave as is.

[NicolasHug]

This has the same level of heading as the table above. Should it be instead at the same level as "classification", "detection", "segmentation" sections?

[jdsgomes]

I think we should split Object Detection, Instance Segmentation and Person Keypoint Detection. In that case yes, this Keypoint detection would be at the same level as "classification", "detection", "segmentation".

• Classification
  • Table of all available Classification weights
    ...
• Object Detection
  • Table of all available detection weights
• Keypoint detection
  • Table of all available detection weights

Otherwise we can keep them in the same group but the relative hierarchy need to change:

• Object Detection, Instance Segmentation and Person Keypoint Detection
  • Object Detection
    • Table of all available detection weights
  • Keypoint detection
    • Table of all available detection weights

[NicolasHug]

Thanks for the input @jdsgomes , that sounds good.

The currently title is Object Detection, Instance Segmentation and Person Keypoint Detection. Do we actually have Instance Segmentation models ATM?

[jdsgomes]

I don't think so

[oke-aditya]

hold on?! Isn't mask rcnn instance segmentation?

[NicolasHug]

Thanks @oke-aditya you're right. As you also properly noted offline, mask-rcnn has a custom mask_map metric, so it would make sense for it to have its own table. Would you like to submit a PR to fix this?

Thinking about our layout again, I guess it could look like

Object Detection, Instance Segmentation and Person Keypoint Detection 
  Object Detection
     table
  Instance segmentation
     table
  Keypoint detection
     table

[oke-aditya]

yep, will send a PR :)

[jdsgomes]

LGTM, thanks for making the changes!

[github-actions]

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