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.

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

Deeplabv3 model builders accept unused **kwargs parameter. #5883

Closed
NicolasHug opened this issue Apr 26, 2022 · 4 comments
Closed

Deeplabv3 model builders accept unused **kwargs parameter. #5883

NicolasHug opened this issue Apr 26, 2022 · 4 comments

Comments

@NicolasHug
Copy link
Member

Not sure if this is by design or a minor bug, but the deeplabV3 model builders accept a **kwargs parameter and do not use it.

https://github.com/pytorch/vision/blob/d425f007782051ab08e9c0fbfe06b072313f4649/torchvision/models/segmentation/deeplabv3.py#L222:L222

@datumbox should we remove these?

@datumbox
Copy link
Contributor

I believe this is done across all model builders and kept there for consistency (all other builders do it). For example lraspp_mobilenet_v3_large makes use of it to handle extra params coming from our reference scripts pass parameters.

@NicolasHug
Copy link
Member Author

Thanks for the context.

Do you think it still makes sense to keep **kwargs for deeplabv3 even though it is never used? It is used in lraspp_mobilenet_v3_large because this one doesn't handle the aux_loss parameter, but the deeplabv3 builder can properly handle all inputs that come from the call you mentioned above.

@datumbox
Copy link
Contributor

Good question. Obviously here the only argument for keeping it is to be consistent with the other model builders.

Every other model builder supports is because it redirects the params to the model class. In segmentation we dont do it because the model classes don't receive too many extra parameters.

I see 2 options:

  1. Go for consistency. Leave the kwargs be but also pass them on the segmentation model classes.
  2. Remove them as they are unnecessary for deeplabv3.

Both have cons. For example in (1) will lead to putting unnecessary kwargs params in the model classes while in (2) one can argue that removing it is not BC.

Frankly I don't have a strong opinion over that. Happy to go with whatever you decide.

@pmeier
Copy link
Collaborator

pmeier commented Apr 27, 2022

Cross linking #5001 (comment) since we had this discussion before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants