Ignite: Add ability to add class name for metrics

[erezalg]

Hello,

I have a code that does classification on CIFAR dataset.

When I use the Recall metric, what I get is a graph of per-class recall, but the legend gives me the label enum of the class and not the name.
It would be great to be able to give a list of class names to such metric so it'll be reported with the name.

[vfdev-5]

@erezalg thanks for the FR. Let us see what can be done for that, either from metrics side or logger side. On your opinion, where it would be more natural to specify that ?

[erezalg]

@vfdev-5
Since this might be relevant for all metrics, I think maybe in the create_supervised_evaluator function call?
This way no need to send it to each specific metric separately. Not sure what it will mean for custom metrics (for legacy support they'll have to ignore this by default).

Otherwise, you can do it in the Metrics' constructor, but it's a bit of a shame to send it multiple times for different metrics.

[sdesrozis]

@erezalg Your point is related to respect RAII principle or not. I prefer configure objects by ctor but I understand that it might be more verbose and quite repetitive in that case.

[erezalg]

@sdesrozis, Sorry, not sure how it's related to RAII, can you elaborate?
I think it's a matter of how it's easier to implement and maintain, from a user standpoint, I prefer to have a single list of class names and send it once to something that is mutual to all the metrics that this list is related to. That was my point.

[sdesrozis]

RAII means you handle initialized objects.

names = [...]
m = MyMetric(names, ...)

If the class names list is not used by metric's ctrs, it means the behaviour of the object can change dynamically.

names = [...]
m = MyMetric(...)
# m does not handle names, if it is dumped, no name used.
m.set_names(names)
# now names are handled, the behaviour has changed.

Ok, the object is usable but not completely defined. RAII aims to avoid that.

Btw, this is a matter of taste. IMO it is easier to maintain and implement because it comes from design and pattern concepts. For the user side, I don't see any difference.

If we want to automate the handling of names in multiple metrics, I'm afraid that do it in create_supervised_evaluator should not be a good idea. Indeed, the method suggests that it aims to create an evaluator. If metric's behaviour is changed silently, it's not clear for the user neither for debug.

Again, what I say here is my point of view. IMO we should really take care about how and where we implement such features.

[erezalg]

@sdesrozis
Now I get it and I think you're right that create_supervised_evaluator might not be a good place because it does stuff to objects it receives "behind the back".
So this kinda leaves the Metric's CTOR as the place to get it, unless there's something else that ties all metrics together

[sdesrozis]

Maybe we should write snippets to design an api especially with several metrics. What do you think ?

[erezalg]

How would you do it?
classes = ['cat', 'dog', 'horse]
r = Recall()
p = Precision()
Metrics.inject_classes(r,p)

Is that what you had in mind?

[sdesrozis]

That's an idea but what means inject_classes for regression metrics which inherits from Metric ?

I prefer simpler and more straightforward solution.

classes = ['cat', 'dog', 'horse]
r = Recall(class_names=classes)
p = Precision(class_names=classes)

Pro : so easy and natural to user
Cons : class_names is repeated

That's just a suggestion for discussion ! Feel free to challenge it and propose something better !

Thank you 😉

[erezalg]

Yeah that would be the obvious one and the easiest to implement (I guess).
BTW, would this cause code duplication in multiple classes as you have to implement handling logic in multpile metrics? or is it part of the parent class's logic?

[sdesrozis]

There is a base class for classification metric _BaseClassification

https://github.com/pytorch/ignite/blob/3bde7329757a7560d7cd0e63f566875a7701d74b/ignite/metrics/accuracy.py#L11

Maybe this could be a suitable place. Metrics Recall, Accuracy and Precision should be improved. Fbeta should be fixed, maybe TopKCategoricalAccuracy.

What do you think ?

[erezalg]

Yeah, this seems reasonable. I can't think of any other metric or use case that I'd need for myself.

[sdesrozis]

@erezalg Could you do a PR with this feature ? It should be a great help !

[erezalg]

@sdesrozis
Sorry for the long silence :) I did not have time to work on this improvement.
I'd be glad to issue a PR and I'm working on modifying the code.

One thing I'm not 100% clear is how do I get to the metrics themselves from an engine object.
What I do is standard:
val_metrics = {"accuracy": Accuracy(),"loss": Loss(criterion), "recall": Recall(labels=classes)}
evaluator = create_supervised_evaluator(net, metrics=val_metrics, device=device)

Where I've added labels parameter to the _BaseClassification class (as well as _BasePrecisionRecall and Recall)
but the only thing I'm able to get is engine.state.metrics (which are the values), and I couldn't figure out how to get to the metric object attached to the engine.

Am I approaching this correctly? if so, any help would be cool!