Unify input checks on detection models

[mmcenta]

Follows up on #2207, unifies checks of the training targets in detection models, and adds checks for all fields (boxes and labels for all models, masks for MaskRCNN, and keypoints for KeypointRCNN).

I followed @fmassa's suggestion to move the validation inside the roi_heads.py file, which already contained a few checks. I merged the existing validation and added functionality.

Note that this can still be improved: I do not check that the masks have the right width and height or that the number of keypoints matches what the model expects.

Importantly, this may be a breaking change because I am validating the dtype of the masks field (which is supposed to be torch.uint8, according to the documentation), which was not done before.

[fmassa]

Thanks for the PR!

Tests are failing (probably due to torchscript limitations), could you have a look?

Let me know if you have questions.

[mmcenta]

Hello,

I investigated the checks that are failing. There are three fails due to torch.dtype not being recognized, and a fourth one is on test_models_detection_negative_samples.py. It happens because the private method that creates the empty samples is creating boxes/masks with N=0 (first dimension), but for some reason, N is hardcoded to 17 for keypoints:

vision/test/test_models_detection_negative_samples.py

Lines 14 to 30 in 2cfc360

	def _make_empty_sample(self, add_masks=False, add_keypoints=False):
	images = [torch.rand((3, 100, 100), dtype=torch.float32)]
	boxes = torch.zeros((0, 4), dtype=torch.float32)
	negative_target = {"boxes": boxes,
	"labels": torch.zeros(0, dtype=torch.int64),
	"image_id": 4,
	"area": (boxes[:, 3] - boxes[:, 1]) * (boxes[:, 2] - boxes[:, 0]),
	"iscrowd": torch.zeros((0,), dtype=torch.int64)}
	if add_masks:
	negative_target["masks"] = torch.zeros(0, 100, 100, dtype=torch.uint8)
	if add_keypoints:
	negative_target["keypoints"] = torch.zeros(17, 0, 3, dtype=torch.float32)
	targets = [negative_target]
	return images, targets

Because the code I wrote checks that N must match for all items in targets, it is throwing an exception.

I think that maybe @fmassa got confused and swapped the first and second dimensions, but I would like your opinion before making the change!

[fmassa]

@mmcenta the test is correct, see

vision/references/detection/coco_utils.py

Line 80 in 2cfc360

keypoints = keypoints.view(num_keypoints, -1, 3)

So keypoints should have the first dimension to be the number of keypoints (always 17 for the curent COCO model, but let's not hard-code it), and the number of objects in the second dimension. So just change the check to be in the second dimension of the keypoints.

For the dtype, is it still an issue? as a workaround you can add a type annotation as an int.

[fmassa]

Actually, looking at this again, there might be a possible improvement in

vision/references/detection/coco_utils.py

Lines 76 to 80 in 2cfc360

	keypoints = [obj["keypoints"] for obj in anno]
	keypoints = torch.as_tensor(keypoints, dtype=torch.float32)
	num_keypoints = keypoints.shape[0]
	if num_keypoints:
	keypoints = keypoints.view(num_keypoints, -1, 3)

that we can do.

Instead of only resizing if the number objects is non-zero, I think we could instead do something like

            keypoints = [obj["keypoints"] for obj in anno]
            keypoints = torch.as_tensor(keypoints, dtype=torch.float32)
            num_keypoints = keypoints.shape[0]
            num_objects = boxes.shape[0]
            keypoints = keypoints.view(num_keypoints, num_objects, 3)

This way the code is more robust. Can you also make a separate PR for this? It would be slightly more robust I think.

[mmcenta]

@fmassa Oh I see, sorry!

I was basing my checks on the documentation which states that the shape of the keypoints vector is (N, K, 3). If you want I can open another PR to fix it.

Yes, CircleCI is still complaining about the dtype, I think I will make the changes to the checks and let the tests run. If they fail again, I will revert back to int. Is that ok?

[fmassa]

@mmcenta looking at it again, I was wrong :-)

The shape of the keypoints is indeed [N, K, 3] I believe. so if you could fix the tests, it would be great!

[mmcenta]

@fmassa Ok I will commit the changes.

Are you sure? What about the code in coco_utils.py with the different order?

[fmassa]

Are you sure? What about the code in coco_utils.py with the different order?

I think I have misread the implementation before because of a bad variable name (num_keypoints1), because keypoints.shape[0]` is the number of annotations (objects).

[fmassa]

Also, can you rebase your changes on top of latest master? There seems to be conflicts.

[mmcenta]

@fmassa Sure, and sorry for all the commits, I need to figure out what CircleCI is complaining about before I can fix it. Before it was casting an Optional[str] to bool, now it is the any builtin (which I should have imagined wouldn't be allowed because there you used a method instead of all).

I will keep doing this until everything is fixed and then squash the commits and rebase to the new master :P

[mmcenta]

@fmassa I'm having trouble when comparing an Optional[int] to an int. The refining wasn't working in the same if statement, so I separated the None check in its own if statement, which also didn't work. Now I am refining via the assert and I still get errors. Do you have an idea of how we fix it?

[fmassa]

@mmcenta I think you need to do something like

item = shape[i]
assert item is not None

[fmassa]

So there seems to be an issue with CI today #2316
Don't worry about failing tests, let me try to understand what is going on with CI and then we can continue checking what the issues are.

But there are some conflicts on your branch that could be fixed already.

[mmcenta]

@fmassa I think the checks that are failing now are not my fault (finally). Also sorry for the mess with the commit I think I messed up the rebase the first time around.

[fmassa]

Hi @mmcenta

Sorry for the delay in getting back to you.

Can you rebase your PR on top of master?
Just doing git pull origin master (where origin points to github.com/pytorch/vision) should do it.

[mmcenta]

Done! There are some failing checks but I don't think it is a consequence of this PR.

[NicolasHug]

Hi @mmcenta , thanks for your work so far! Would you be interested in continuing this PR? There are a few linting issues, and merging with master should hopefully fix the unrelated CI issues, which would help for reviews. Thanks!