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.

Sign up for GitHub

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

Allow variable number of repetitions for RA #5084

Merged
merged 2 commits into from
Dec 20, 2021
Merged

Allow variable number of repetitions for RA #5084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6d4868
Allow variable number of repetitions for RA
tbennun Dec 10, 2021
9a89f9d
Merge branch 'main' into ra-reps
datumbox Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions references/classification/sampler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class RASampler(torch.utils.data.Sampler):
https://github.com/facebookresearch/deit/blob/main/samplers.py
"""

def __init__(self, dataset, num_replicas=None, rank=None, shuffle=True, seed=0):
def __init__(self, dataset, num_replicas=None, rank=None, shuffle=True, seed=0, repetitions=3):
if num_replicas is None:
if not dist.is_available():
raise RuntimeError("Requires distributed package to be available!")
Expand All @@ -28,11 +28,12 @@ def __init__(self, dataset, num_replicas=None, rank=None, shuffle=True, seed=0):
self.num_replicas = num_replicas
self.rank = rank
self.epoch = 0
self.num_samples = int(math.ceil(len(self.dataset) * 3.0 / self.num_replicas))
self.num_samples = int(math.ceil(len(self.dataset) * float(repetitions) / self.num_replicas))
self.total_size = self.num_samples * self.num_replicas
self.num_selected_samples = int(math.floor(len(self.dataset) // 256 * 256 / self.num_replicas))
self.shuffle = shuffle
self.seed = seed
self.repetitions = repetitions

def __iter__(self):
# Deterministically shuffle based on epoch
Expand All @@ -44,7 +45,7 @@ def __iter__(self):
indices = list(range(len(self.dataset)))

# Add extra samples to make it evenly divisible
indices = [ele for ele in indices for i in range(3)]
indices = [ele for ele in indices for i in range(self.repetitions)]
indices += indices[: (self.total_size - len(indices))]
assert len(indices) == self.total_size

Expand Down
7 changes: 5 additions & 2 deletions references/classification/train.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ def load_data(traindir, valdir, args):
print("Creating data loaders")
if args.distributed:
if args.ra_sampler:
train_sampler = RASampler(dataset, shuffle=True)
train_sampler = RASampler(dataset, shuffle=True, repetitions=args.ra_reps)
else:
train_sampler = torch.utils.data.distributed.DistributedSampler(dataset)
test_sampler = torch.utils.data.distributed.DistributedSampler(dataset_test, shuffle=False)
Expand Down Expand Up @@ -485,7 +485,10 @@ def get_args_parser(add_help=True):
"--train-crop-size", default=224, type=int, help="the random crop size used for training (default: 224)"
)
parser.add_argument("--clip-grad-norm", default=None, type=float, help="the maximum gradient norm (default None)")
parser.add_argument("--ra-sampler", action="store_true", help="whether to use ra_sampler in training")
parser.add_argument("--ra-sampler", action="store_true", help="whether to use Repeated Augmentation in training")
parser.add_argument(
"--ra-reps", default=3, type=int, help="number of repetitions for Repeated Augmentation (default: 3)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on whether we should have both a boolean and an integer param VS combining the two a single? Other parameters (such as --clip-grad-norm, --auto-augment) take the latter approach by defining default=None or default=0 to turn off the feature by default. Though having an extra boolean option is useful when you have many config parameters for the feature, it also creates noise when reading the training log to determine which inputs were actually active for the training.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely agree, though I followed the style of the EMA flag(s). Should I switch to a more general --repeated-augmentations with default 0?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the input. :) When we introduced EMA, we offered 2 parameters --model-ema-steps and --model-ema-decay that's why we didnt follow this approach.

@sallysyw Any thoughts on which approach we should choose here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Two things to consider here:

  1. Users may not be aware of what is a "good value" for the number of repetitions. This is solvable though: it could be given as an example in the help of the argument.
  2. In my fork, I'm adding another boolean flag to keep the same epoch length (rather than cutting it down by the number of repetitions) and replicate the samples within the same minibatch (and GPU), following the original Batch Augmentation paper. If it improves generalization more than the DeiT approach, would you be interested in this as part of another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Concerning point 1, I agree with your arguments and I don't have strong opinions. I think your PR is mergeable as-is but I'll let @sallysyw who contributed the class to decide whether we should go with 2 parameters or 1.

Concerning point 2, unless the gap in accuracy is massive, I would keep things simple and leave it as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I missed the discussion before. I think for this specific usage case, having 2 parameters for RA are better than 1 because, as @tbennun mentioned, users might don't have a good sense on which ra_reps to use. So it's important to have a non-zero default (also for the backward compatibility of replicating ViT training).

)

# Prototype models only
parser.add_argument("--weights", default=None, type=str, help="the weights enum name to load")
Expand Down