RFC on schema for the `metrics` field of weights meta-data

[NicolasHug]

Problem

We currently store the metrics as a simple dictionary metric -> value. For classification weights this looks like this:

vision/torchvision/models/resnet.py

Lines 320 to 323 in 7d0d7fd

	"metrics": {
	"acc@1": 69.758,
	"acc@5": 89.078,
	},

and the underlying assumption is that the dataset is ImageNet.

But in the future, we might want to provide metrics for more than one dataset, not just ImageNet. Our current schema doesn't allow us to do that nicely. In fact we already have such an problem with the Optical Flow weights, which typically report EPE on different datasets, and keys of the metrics dict currently look like:

sintel_train_cleanpass_epe                                                     
sintel_train_finalpass_epe                                                                                                                
sintel_test_cleanpass_epe                                                      
sintel_test_finalpass_epe                   

which is pretty ugly.

Proposed schema

There's a million ways to structure this. I guess the simplest one is to have a metrics dict with such structure:

"metrics": { 
    "ImageNet": {
        "acc@1": 69.758, 
         "acc@5": 89.078, 
    },
    "ImageNetV2": {
        "acc@1": 99.999, 
         "acc@5": 0.0001, 
    },
 },

This makes it slightly less convenient to access values programatically, but I'm wondering whether this is actually something we should encourage?

I'm almost tempted to rename the "metrics" key of the meta dict into "_metrics" to strongly discourage users to use it, and to allow us to change the schema in the future if we find a need for it. Happy to hear your thoughts

[datumbox]

@NicolasHug Thanks for the write up. I think your proposal is reasonable.

An alternative discussed earlier (see #5859 (comment)) was to provide both a main acc@1 metric which will serve as the "Default" reported accuracy and a breakdown of accuracies on other datasets if available. Using your current lingo this would look like:

"metrics": {
    # Default mandatory metrics
    "acc@1": 69.758, 
    "acc@5": 89.078, 
    # Optional breakdown per dataset, can be considered private
    "_details": {
        "ImageNet": {  # Duplicated information
             "acc@1": 69.758, 
             "acc@5": 89.078, 
        },
        "ImageNetV2": {
             "acc@1": 99.999, 
             "acc@5": 0.0001, 
        },
    },
 },

There are pros/cons with the above approach. On the pros side, it will be easier to programmatically access the mandatory metric fields because the user will be guaranteed to find meta["metrics"]["acc@1"]. This can be enforced with unit-tests. On the cons side, the above has a degree of info duplication.

Concerning making _metrics private, I find this idea tempting. One concern I have is that we already use it programmatically on our documentation, so perhaps other users might want to do something similar. This is why above I'm making the _details field private as a middle ground.

I don't have very strong opinions over this. Both options have pros/cons and I'm not 100% sold on any of them. I would love to hear what others think.

[datumbox]

@NicolasHug Could you provide an example of how you plan to structure under your proposal the epe metrics for sintel_[train|test]_[clean|final]pass?

[NicolasHug]

@NicolasHug Could you provide an example of how you plan to structure under your proposal the epe metrics for sintel_[train|test]_[clean|final]pass?

Basically replace "ImageNet[V2]" with one of

"sintel_train_cleanpass", "sintel_train_finalpass", "sintel_test_cleanpass", "sintel_test_finalpass".

We can add yet another level/dict for the split, and for the pass, and in general we can add as many level as there are parameters in the dataset constructor (kinda). But it's not obvious how useful that would be.

Clearly there's not obvious good solution here, and our indecisiveness makes me think we should just keep all this mess private.

[NicolasHug]

Regarding the "default" metric discussion: I'm probably missing something, but right now I don't see what use-case this would address. I have concerns that this might lead to incorrect comparisons (typically on the optical flow datasets where most of the weights aren't comparable with one-another), but perhaps there are good reasons to have these. Unfortunately for now the advantage isn't clear to me

[datumbox]

@jdsgomes @YosuaMichael Let's hear your thoughts on the proposal and finalize/implement this by mid next week. We are getting closer to the release so this needs to be decided soon. :)

[YosuaMichael]

I think the proposal to specify datasets for the metric looks good.

For the default value, I have no strong opinion on this, the value that I can think of is to be BC. However if we add default value, I think it should be clear which dataset that is used for the default value (either documented somewhere else, or we can specify with "default_dataset": "ImageNet").

I am in support of making metrics private, because I think this is not the main purpose of torchvision as library hence I think it is okay to not give BC support for this.

[jdsgomes]

IMO the cleanest option is the initially proposed by @NicolasHug. In that case the changes won't be BC, so I would take the opportunity to make them "private" at the same time.

[datumbox]

I'm OK adopting @NicolasHug proposal. I don't think it's necessary to specify a default dataset or a default value. I would be more inclined to keep the metrics public thought as it would be useful for downstream projects (Papers with code repo, D2go Zoo, etc) to programmatically read the values but I'm OK if we want to release it first as private until we are certain about the schema.

@NicolasHug are you unblocked to implement your proposal?

[datumbox]

QQ:
Partially "thanks" to my comment at #6047 (comment), we've ended up adopting capitalized dataset names with - and _. Those look nicer in the docs but, after a good night's sleep, I'm starting questioning that proposal because literally every other key in this dictionary is lower-cased and with _.

Given that Nicolas has turned the _metrics field private, it's not a big deal to leave as-is and change on the future. Alternatively, I could bring a PR that changes this now. Thoughts @NicolasHug @jdsgomes @YosuaMichael?

[NicolasHug]

I don't have a strong opinion.

At this point my preferred solution would actually to not use - or _, but instead write purely human-readable dataset names like ImageNet 1K or Sintel (test), clean pass. It would make it a bit more awkward to access programmatically by users, which IMHO is a good thing.

But again, no strong opinion. Using - vs _ feels the same to me, so if you submit a PR I'll approve it.

[YosuaMichael]

No strong opinion on the dataset naming for the docs as well. I think @NicolasHug idea on having human-readable dataset also make sense for docs :)