get_coco_api_from_dataset returns wrong dataset

[yumanets]

🐛 Bug

I tried to train a detector on a custom dataset (using https://pytorch.org/tutorials/intermediate/torchvision_tutorial.html) and the training is stuck on 60% of precision. So I started looking at the source code and noticed that get_coco_api_from_dataset() is probably doing the wrong thing. In case the Subset is used for train and test datasets, it returns its original dataset. Problem is that it returns the entire dataset, not just the test dataset, which is the part of the entire dataset. This results in slow down when building a needless full Coco dataset for evaluation. It also updates coco_evaluator in evaluate() with wrong stats because of conflicting "image_id". According to the tutorial "image_id" is unique inside one data set and this is why the dataset "index" is used. This all results in invalid evaluation stats presented during training.

To Reproduce

Steps to reproduce the behavior:
No reproducing is needed.

@torch.no_grad()
def evaluate(model, data_loader, device):
...
---> coco = get_coco_api_from_dataset(data_loader.dataset) # this returns full dataset
...
res = {target["image_id"].item(): output for target, output in zip(targets, outputs)}
evaluator_time = time.time()
---> coco_evaluator.update(res) # invalid target is updated
...
return coco_evaluator

Expected behavior

Subset dataset should be used in get_coco_api_from_dataset()

Environment

Collecting environment information...
PyTorch version: 1.6.0
Torchvision: 0.7.0
Is debug build: N/A
CUDA used to build PyTorch: N/A

OS: Ubuntu 20.04 LTS (x86_64)
GCC version: (Ubuntu 9.3.0-10ubuntu2) 9.3.0
Clang version: Not used
CMake version: version 3.16.3

Python version: 3.8 (64-bit runtime)
Is CUDA available: yes
CUDA runtime version: 11.0.2-1
GPU models and configuration: GPU 0: GeForce GTX 1080 Ti
Nvidia driver version: 450.51.05
cuDNN version: /usr/lib/x86_64-linux-gnu/libcudnn.so.7.4.2

Versions of relevant libraries:
[pip3] numpy==1.17.4
[conda] Not used

[fmassa]

Hi,

Thanks for opening the issue. I see that you already closed it, would you like to comment a bit on it?

We could indeed make the get_coco_api_from_dataset more generic by only considering the subset, similar to what we do for the aspect ratio computation in

vision/references/detection/group_by_aspect_ratio.py

Lines 154 to 176 in fc69c22

	def _compute_aspect_ratios_subset_dataset(dataset, indices=None):
	if indices is None:
	indices = range(len(dataset))
	ds_indices = [dataset.indices[i] for i in indices]
	return compute_aspect_ratios(dataset.dataset, ds_indices)
	def compute_aspect_ratios(dataset, indices=None):
	if hasattr(dataset, "get_height_and_width"):
	return _compute_aspect_ratios_custom_dataset(dataset, indices)
	if isinstance(dataset, torchvision.datasets.CocoDetection):
	return _compute_aspect_ratios_coco_dataset(dataset, indices)
	if isinstance(dataset, torchvision.datasets.VOCDetection):
	return _compute_aspect_ratios_voc_dataset(dataset, indices)
	if isinstance(dataset, torch.utils.data.Subset):
	return _compute_aspect_ratios_subset_dataset(dataset, indices)
	# slow path
	return _compute_aspect_ratios_slow(dataset, indices)

But I'm not sure if this is necessary for Subset, because IIRC the current evaluation code handles this correctly.

[mattbwilmes]

@fmassa I just ran into this issue as well while roughly following https://pytorch.org/tutorials/intermediate/torchvision_tutorial.html. The issue I am seeing is using torch.utils.data.Subset. For example, I have the following code:

dataset_test = torch.utils.data.Subset(dataset, indices[50:100])
data_loader_test = torch.utils.data.DataLoader(
    dataset_test, batch_size=1, shuffle=False, num_workers=4,
    collate_fn=utils.collate_fn)
get_coco_api_from_dataset(data_loader_test.dataset)

This is triggering

vision/references/detection/coco_utils.py

Lines 202 to 203 in 13b35ff

	if isinstance(dataset, torch.utils.data.Subset):
	dataset = dataset.dataset

and therefore calling return convert_to_coco_api(data_loader_test.dataset.dataset). However, len(data_loader_test.dataset.dataset) is 13000+ (my entire dataset).

I'm guessing there was a reason for this isinstance() check, but in my case I just want to have it call return convert_to_coco_api(data_loader_test.dataset)

[fmassa]

Yes, this is a known simplification, and we could make it truly consider the subset of the code.

IIRC the reason why I did this simplification was that if we had a subset of a COCO dataset, we didn't need to iterate over the sub dataset to create the index, as it was already done, saving some time during early development.
But this code is fundamentally wrong and we should fix it.