-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Adding multiweight support to FasterRCNN #4847
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.
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
Adding multiweight support to FasterRCNN #4847
Conversation
💊 CI failures summary and remediationsAs of commit 46f30c3 (more details on the Dr. CI page):
2 failures not recognized by patterns:
This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding few comments:
@@ -33,6 +33,12 @@ | |||
from group_by_aspect_ratio import GroupedBatchSampler, create_aspect_ratio_groups | |||
|
|||
|
|||
try: | |||
from torchvision.prototype import models as PM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily adding support of prototype on the detection reference script similar to all other areas, so we can test the results.
x = torch.rand(input_shape).to(device=dev) | ||
if module_name == "detection": | ||
x = [x] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Detection models receive a list of images.
@@ -162,7 +162,7 @@ def _shufflenetv2(arch: str, pretrained: bool, progress: bool, *args: Any, **kwa | |||
if pretrained: | |||
model_url = model_urls[arch] | |||
if model_url is None: | |||
raise NotImplementedError(f"pretrained {arch} is not supported as of now") | |||
raise ValueError(f"No checkpoint is available for model type {arch}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an unrelated correction, to align the exception type and value with the entire TorchVision so that we can capture it easier.
if "pretrained_backbone" in kwargs: | ||
warnings.warn("The argument pretrained_backbone is deprecated, please use weights_backbone instead.") | ||
weights_backbone = MobileNetV3LargeWeights.ImageNet1K_RefV1 if kwargs.pop("pretrained_backbone") else None | ||
weights_backbone = MobileNetV3LargeWeights.verify(weights_backbone) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will eventually be refactored to reduce duplicate code but that's part of the clean up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some small comments, let me know your thoughts. Otherwise looks good.
@@ -41,8 +47,15 @@ def get_dataset(name, image_set, transform, data_path): | |||
return ds, num_classes | |||
|
|||
|
|||
def get_transform(train, data_augmentation): | |||
return presets.DetectionPresetTrain(data_augmentation) if train else presets.DetectionPresetEval() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: would it be clearer to have weights as explicit argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to avoid polluting massive parts of the references with weights
references while pretrained
is supported concurrently. Are you OK if we do this as part of the cleanup operations recorded at #4652?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Summary: * Aligning exception with all other models. * Adding prototype preprocessing on video references. * Adding the rest of model builders on faster_rcnn. Reviewed By: kazhang Differential Revision: D32216664 fbshipit-source-id: 6998018eebe4381b9bff9e9290061d35e42faa21
* Aligning exception with all other models. * Adding prototype preprocessing on video references. * Adding the rest of model builders on faster_rcnn.
Fixes partially #4675
Verified that both commands return the same:
cc @datumbox @bjuncek