-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Use Kinetics instead of Kinetics400 in references (#5787) #5952
Changes from all commits
a98c056
c5c36d8
2ee355a
5f90f09
5559593
01b0d9b
a84b807
6d2f7f8
9d76b18
2d2841f
e8401c1
11f365d
8c05f0f
ab29225
af4e30c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,8 +130,8 @@ def main(args): | |
|
||
# Data loading code | ||
print("Loading data") | ||
traindir = os.path.join(args.data_path, args.train_dir) | ||
valdir = os.path.join(args.data_path, args.val_dir) | ||
traindir = os.path.join(args.data_path, "train") | ||
valdir = os.path.join(args.data_path, "val") | ||
|
||
print("Loading training data") | ||
st = time.time() | ||
|
@@ -145,9 +145,11 @@ def main(args): | |
else: | ||
if args.distributed: | ||
print("It is recommended to pre-compute the dataset cache on a single-gpu first, as it will be faster") | ||
dataset = torchvision.datasets.Kinetics400( | ||
traindir, | ||
dataset = torchvision.datasets.Kinetics( | ||
args.data_path, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing this is done, because we can now select the split through the dataset class rather than using a different root directory, correct? If yes, can we remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup; and done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pmeier @bjuncek I checked on the servers, I believe there is a reason why these parameter are there. The datasets contain the training/val data at different preprocessed sizes. Example:
Can we please restore this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed with @datumbox : the different pre-processed size don't seem to be part of the original downloadable dataset. Looks more like an internal pre-processing that was made by someone. IF someone was relying on this originally, they should have gotten deprecation warning telling them to use Kinetics, and they should have noticed that this wasn't supported anymore, which they should have told us. This never happened, so we can assume this support for the different sizes isn't actually needed. If it is, we'll know soon enough. So we're OK to remove that. |
||
frames_per_clip=args.clip_len, | ||
num_classes=args.kinetics_version, | ||
split="train", | ||
step_between_clips=1, | ||
transform=transform_train, | ||
frame_rate=15, | ||
|
@@ -179,9 +181,11 @@ def main(args): | |
else: | ||
if args.distributed: | ||
print("It is recommended to pre-compute the dataset cache on a single-gpu first, as it will be faster") | ||
dataset_test = torchvision.datasets.Kinetics400( | ||
valdir, | ||
dataset_test = torchvision.datasets.Kinetics( | ||
args.data_path, | ||
frames_per_clip=args.clip_len, | ||
num_classes=args.kinetics_version, | ||
split="val", | ||
step_between_clips=1, | ||
transform=transform_test, | ||
frame_rate=15, | ||
|
@@ -312,8 +316,9 @@ def parse_args(): | |
parser = argparse.ArgumentParser(description="PyTorch Video Classification Training") | ||
|
||
parser.add_argument("--data-path", default="/datasets01_101/kinetics/070618/", type=str, help="dataset path") | ||
parser.add_argument("--train-dir", default="train_avi-480p", type=str, help="name of train dir") | ||
parser.add_argument("--val-dir", default="val_avi-480p", type=str, help="name of val dir") | ||
parser.add_argument( | ||
"--kinetics-version", default="400", type=str, choices=["400", "600"], help="Select kinetics version" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to update the commands shown at README.md. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
) | ||
parser.add_argument("--model", default="r2plus1d_18", type=str, help="model name") | ||
parser.add_argument("--device", default="cuda", type=str, help="device (Use cuda or cpu Default: cuda)") | ||
parser.add_argument("--clip-len", default=16, type=int, metavar="N", help="number of frames per clip") | ||
|
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.
Why can we simply remove this? It seems something is off here. The old
Kinetics400
statesvision/torchvision/datasets/kinetics.py
Line 292 in 970ba35
whereas the new
Kinetics
statesvision/torchvision/datasets/kinetics.py
Line 69 in 970ba35
Was this intentional or did we botch this during the introduction of the new class? In any way, we need to adapt our deprecation warnings accordingly:
vision/torchvision/datasets/kinetics.py
Lines 252 to 254 in 970ba35
vision/torchvision/datasets/kinetics.py
Lines 308 to 311 in 970ba35
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.
This was intentional -- the first iteration was convenient because that's the raw output from FFMPEG/pyav, but tv ops like transforms and conv operators work on Tensor[T, C, H, W] (as you've seen you'd have to have a transform to deal with this). The change makes sense, and I'll update the deprication warning, how does that sound?