Skip to content

Adding args for names of train and val directories #1544

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

Merged
merged 3 commits into from
Nov 26, 2019

Conversation

rsomani95
Copy link
Contributor

As discussed in #1540

@codecov-io
Copy link

codecov-io commented Oct 31, 2019

Codecov Report

Merging #1544 into master will increase coverage by 0.88%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1544      +/-   ##
==========================================
+ Coverage    64.6%   65.49%   +0.88%     
==========================================
  Files          87       90       +3     
  Lines        6742     7078     +336     
  Branches     1034     1076      +42     
==========================================
+ Hits         4356     4636     +280     
- Misses       2085     2134      +49     
- Partials      301      308       +7
Impacted Files Coverage Δ
torchvision/models/quantization/__init__.py 100% <0%> (ø) ⬆️
torchvision/datasets/video_utils.py 72.25% <0%> (ø) ⬆️
torchvision/models/quantization/inception.py 83.05% <0%> (ø)
torchvision/models/quantization/googlenet.py 65.9% <0%> (ø)
torchvision/models/quantization/shufflenetv2.py 90.16% <0%> (ø)
torchvision/models/shufflenetv2.py 85.88% <0%> (+0.16%) ⬆️
torchvision/models/inception.py 87.67% <0%> (+2.32%) ⬆️
torchvision/models/googlenet.py 76.57% <0%> (+3.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca57148...85e3f8d. Read the comment docs.

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I think there is a problem with how you are handling the different number of classes. Apart from that, the other changes are good to me, so if you would want to get this merged quickly, just removing the output_classes part of the code would be fine with me

@@ -203,6 +203,8 @@ def main(args):

print("Creating model")
model = torchvision.models.video.__dict__[args.model](pretrained=args.pretrained)
if args.output_classes is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This does not affect anything in the code. You would need to pass num_classes=output_classes to the video model for it to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I fixed this in the latest commit.

@rsomani95
Copy link
Contributor Author

Fixed it while making the default output_classes = 400, as it made more sense w.r.t the pretrained model.
There was another minor typo that was fixed: args.valid_dir --> args.val_dir

@@ -203,6 +203,7 @@ def main(args):

print("Creating model")
model = torchvision.models.video.__dict__[args.model](pretrained=args.pretrained)
model.fc.out_features = args.output_classes
Copy link
Member

Choose a reason for hiding this comment

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

This is not quite what I meant.

Changing model.fc.out_features doesn't change the number of output planes, because the model weights have already been generated.
What I meant was to do something like

model = torchvision.models.video.__dict__[args.model](pretrained=args.pretrained, num_classes=args.num_classes)

But then, using pretrained=True and num-classes different than 400 will give an error, because the state-dicts are not compatible.

I'd rather not expose this option to the training scripts, as the user can modify the model as they want to perform their type of fine-tuning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If fine-tuning on a custom dataset, one loads in a pretrained model with that model's num_class (400 here), and then changes the output classes to their custom dataset.

Though the output planes are different, the biggest benefit from a pretrained model aren't in the fc part of the model, but the convolutional filters that come in the previous parts of the model. The fc parts get retrained when fine-tuning this way.

I'd rather not expose this option to the training scripts, as the user can modify the model as they want to perform their type of fine-tuning.

This change in model.fc.out_features is mandatory when fine-tuning on a custom dataset, so I thought it would make sense to have it in the args. But of course, that's your call :)

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that this change doesn't actually change anything in the model. The weights are still the same as before, and they are not resized to a different size.

Putting it differently, you'll only predict at most 400 classes in this way.

@bjuncek
Copy link
Contributor

bjuncek commented Nov 21, 2019

@rsomani95 take A look at how Kenso deals with finetuning - it's a fairly elegant approach

ref: https://github.com/kenshohara/3D-ResNets-PyTorch

@rsomani95
Copy link
Contributor Author

@bjuncek Thanks for the reference, I'll take a look.
@fmassa I'm a bit tied down with other work for now, is it alright if I put a hold on the PR for now, or would you rather close it and re-open later?

@fmassa
Copy link
Member

fmassa commented Nov 26, 2019

@rsomani95 Let's close the PR for now, and re-open it when you have the time.
Or, what we could do as well is to remove the controversial changes from this PR (like the number of classes), and just keep the video path.

Thoughts?

@rsomani95
Copy link
Contributor Author

@fmassa That sounds good. Commiting just the video path changes.

@rsomani95 rsomani95 changed the title Generalised video-classification training script for custom dataset Adding args for names of train and val directories Nov 26, 2019
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks!

@fmassa fmassa merged commit a44d55d into pytorch:master Nov 26, 2019
@rsomani95 rsomani95 deleted the video-classification branch November 26, 2019 17:23
fmassa pushed a commit to fmassa/vision-1 that referenced this pull request Dec 18, 2019
* Generalised for custom dataset

* Typo, redundant code, sensible default

* Args for name of train and val dir
facebook-github-bot pushed a commit that referenced this pull request Dec 19, 2019
Summary:
* Generalised for custom dataset

* Typo, redundant code, sensible default

* Args for name of train and val dir
Pull Request resolved: #1683

Reviewed By: cpuhrsch

Differential Revision: D19156990

Pulled By: fmassa

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

Successfully merging this pull request may close these issues.

4 participants