Revert MAP implementation to pycocotools backend

[SkafteNicki]

What does this PR do?

Fixes #1024
Fixes #1164

Reverts the MeanAveragePrecision implementation back to its v0.6 version for two primary reasons:

• Our current implementation is orders of magnitude slower and even with improvements in Improve Mean Average Precision performance #1389 it is still slower
• Nobody on the primary team is actually able to maintain the metric because we do not have experience with object detection. Therefore it is better to rely on pycocotools during the major calculations and we "just" provide an interface

The consequence for now is that pycocotools will be (re-)introduced as an require dependency for the metric.

TODO list:

• Revert implementation
• Make sure implementation supports argument iou_type (introduced after v0.6)
• Make sure implementation supports argument iou_thresholds, rec_thresholds, max_detection_thresholds (introduced after v0.6)
• Benchmark implementation to make sure it is still faster
• Move from static testing to dynamic testing (for most tests) = call pycocotools directly during testing to check if output is correct
• Check if implementation fixes Order of empty images in the batch changes the mAP results #1774
• Check if implementation fixes Over-estimated map value  #1793
• Check if implementation fixes Wrong Calculation of Mean Average Precision #1184
• Check if implementation fixes Discrepancy between values with metrics and detectron2 cocoeval #1164

Benchmark results below (measured on my laptop)

Time	This PR	Master	PR #1389 (improves current implementation)
Total time in init	0.0007	0.0010	0.0012
Total time in update	0.2445	0.2472	0.2769
Total time in compute	10.5286	324.6941	220.8867

As it been stated many times the old implementation is orders of magnitude faster than our current implementation.

Benchmark code

import time

import torch

try:
    from torchmetrics.detection import MeanAveragePrecision
except ImportError:
    from torchmetrics.detection import MAP
    MeanAveragePrecision = MAP

total_time = {}

class UpdateTime:
    def __init__(self, name) -> None:
        self._name = name

    def __enter__(self):
        self._start_time = time.perf_counter()

    def __exit__(self, exc_type, exc_val, exc_tb):
        end_time = time.perf_counter()
        if self._name in total_time:
            total_time[self._name] += end_time - self._start_time
        else:
            total_time[self._name] = end_time - self._start_time
        return True

def generate(n):
    boxes = torch.rand(n, 4) * 1000
    boxes[:, 2:] += boxes[:, :2]
    labels = torch.randint(0, 10, (n,))
    scores = torch.rand(n)
    return {"boxes": boxes, "labels": labels, "scores": scores}

with UpdateTime("init"):
    map = MeanAveragePrecision()

for _batch_idx in range(100):
    with UpdateTime("update"):
        detections = [generate(100) for _ in range(10)]
        targets = [generate(10) for _ in range(10)]
        map.update(detections, targets)

with UpdateTime("compute"):
    map.compute()

for name, time in total_time.items():
    print(f"Total time in {name}: {time}")

Before submitting

• Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

---

📚 Documentation preview 📚: https://torchmetrics--1832.org.readthedocs.build/en/1832/

[Borda]

can we rather preserve both so have MAPCoco?

[SkafteNicki]

can we rather preserve both so have MAPCoco?

Yes, we could do that, but then I would prefer we just add a backend="pycocotools"/"pytorch" argument to the class. That said, I am in the process of improving the testing and if the old implementation does not pass, then I would propose to still remove it.

[Borda]

I would prefer we just add a backend="pycocotools"/"pytorch" argument to the class

yes, that is also a good option. Just feel we have invested lots of effort to have our own implementation, so dropping it... :( also this would unblock users with exactly coco expectations (even if there are some known bugs)

[SkafteNicki]

I would prefer we just add a backend="pycocotools"/"pytorch" argument to the class

yes, that is also a good option. Just feel we have invested lots of effort to have our own implementation, so dropping it... :( also this would unblock users with exactly coco expectations (even if there are some known bugs)

We should not keep it just because we have invested time in it, that is just a sunk cost fallacy. We should keep it because we intent (with help) to make it stable.

[ddelange]

hi 👋 does this PR include breaking API changes?

can we still from torchmetrics.detection.mean_ap import MeanAveragePrecision and the API will be the same as before, just with the pycocotools backend?

[Borda]

can we still from torchmetrics.detection.mean_ap import MeanAveragePrecision and the API will be the same as before, just with the pycocotools backend?

yes, it shall be compatible 🦩
cc: @SkafteNicki

[SkafteNicki]

hi 👋 does this PR include breaking API changes?

can we still from torchmetrics.detection.mean_ap import MeanAveragePrecision and the API will be the same as before, just with the pycocotools backend?

@ddelange, no breaking changes :]

[adamjstewart]

no breaking changes :]

One breaking change :]

The following code:

class Foo(LightningModule):
    def __init__(self):
        self.metric = MeanAveragePrecision()

    def on_validation_epoch_end(self):
        self.log_dict(self.metric.compute())

used to work in 0.11.4. However, this PR adds a new classes key-pair to the metrics dict containing a list of all observed classes. log_dict now complains about this entry:

ValueError: `self.log(classes, tensor([0, 1], dtype=torch.int32))` was called, but the tensor must have a single element. You can try doing `self.log(classes, tensor([0, 1], dtype=torch.int32).mean())`

Here, tensor([0, 1]) is the classes value. Current workaround is to delete this key from the dict before logging, but it would be good to do this automatically.

[ddelange]

That's not this PR, it's a breaking change that was introduced in v1.0.0.rc0 ref ac64e63

[adamjstewart]

Good catch, thanks!

[ddelange]

No worries, I happened to know because I hit it in in a recent PR during a particularly representative case of dephell

[SkafteNicki]

@adamjstewart @ddelange
Yeah, I was probably too fast with saying no breaking changes :/
Would you prefer that we introduced an argument include_classes (or something similar) when initializing the class to enable/disable the inclusion of the class key.

For now you can also get around by a simple subclass:

from torchmetrics.detection.mean_ap import MeanAveragePrecision as MAP
class MeanAveragePrecision(MAP):
    def compute(self):
        res = super().compute()
        res.pop("classes")
        return res

[adamjstewart]

An argument would be good. I would vote for it to default to False so that the above example works out-of-the-box. I can submit a PR if you'd like.

[SkafteNicki]

An argument would be good. I would vote for it to default to False so that the above example works out-of-the-box. I can submit a PR if you'd like.

Please feel free to open a PR. I would prefer if we set the default to be True such that we do not break backwards compatible once again.

[adamjstewart]

If it defaults to True then the above code will continue to not work. The purpose of a PR would be to restore backwards compatibility, not to double down on the change.