Ignite: Metrics general supplementary checking before `compute`

[vfdev-5]

As discussed here, the following assumption:

All metrics that has not been updated yet after last reset should raise NotComputableError when compute is called

holds for any metric.
Today we check metric specific variable _num_examples if it's equal zero and raise exception if it is the case. This behaviour can be replaced directly in Metric by counting the number of update calls between reset and before compute.
An example implementation is proposed by @zasdfgbnm :

class Metric:
    def  __init__(self, ...):
        ......

        def wrapped_reset():
            self._updated = False
            self.reset()
        self.reset = functools.wraps(self.reset)(wrapped_reset)

        def wrapped_update(output):
            self._updated = True
            self.update(output)
        self.update =  functools.wraps(self.update)(wrapped_update)

        def wrapped_compute():
            if not self._updated:
                raise NotComputableError('not updated before compute')
            return self.compute()
        self.compute =  functools.wraps(self.compute)(wrapped_compute)

Let's discuss here what we can do.

cc @zasdfgbnm

[zasdfgbnm]

Just one thing to mention, the above approach requires the subclass call super(MyClass, self).__init__, if forgot, then there will be no check.

Another approach to implement this is by metaclass metaprogramming. Let me think a while and try something to see if it works.

[vfdev-5]

@zasdfgbnm thank you, sounds good ! What do you think about to implement a decorator that counts number of calls of a function (or maybe take an existing implementation from somewhere) ?

[zasdfgbnm]

@vfdev-5 The metaclass approach is here: https://github.com/pytorch/ignite/pull/336, it does not require subclass to explicitly call super().__init__.

[zasdfgbnm]

What do you think about to implement a decorator that counts number of calls of a function (or maybe take an existing implementation from somewhere)

Why counting, instead of just a boolean? I didn't get your point.

[vfdev-5]

Why counting, instead of just a boolean? I didn't get your point.

Yes, just boolean will work too. The point was to use a decorator instead of metaclasses...

[zasdfgbnm]

@vfdev-5 OK, I understand the point now... Decorator vs metaclass, which is the preferred way?

[vfdev-5]

@zasdfgbnm I would say the simplest without new dependencies :)

[zasdfgbnm]

@vfdev-5 actually we can not get rid of new dependency if we still want to set the metaclass of Metric to ABCMeta, the current way in https://github.com/pytorch/ignite/blob/master/ignite/metrics/metric.py#L19 only works for python2...

[zasdfgbnm]

For decorator approach, don't we need to manually decorate all subclasses?

[vfdev-5]

@zasdfgbnm that's true that the current implementation of Metric does not work properly with ABCMeta.

cc @jasonkriss

For decorator approach, don't we need to manually decorate all subclasses?

We can decorate iteration_completed and completed what already wrap update and compute.

[zasdfgbnm]

@vfdev-5 I opened a new issue for the metaclass problem: https://github.com/pytorch/ignite/issues/337

[zasdfgbnm]

@vfdev-5 Hmm... if we decorate iteration_completed and completed, the we are assuming that metrics would only be used with Engine. If this is the case, since Events.EPOCH_COMPLETED will always be fired after Events.EPOCH_STARTED then Events.ITERATION_COMPLETED, so we are guaranteed to have reset->(update)+->compute behavior, there is nothing to check.

[vfdev-5]

@zasdfgbnm yes, in some sense iteration_completed and completed are tightly related to Engine. IMO, methods update, compute are "internal" API, maybe @jasonkriss has another opinion. I understand that we use it almost everywhere when we wrap around a metric instance, e.g. RunningAverage, MetricLambda etc.

I'm hesitating to introduce another class just to cover the logic reset-> (update)+ ->compute.
Let me think about this more.

[zasdfgbnm]

@vfdev-5 Another thing in my mind that I haven't said anywhere yet was to extending the idea of https://github.com/pytorch/ignite/issues/321 to metaclass, I'm not sure if this is a good idea.

What in my mind was, add __add__, __mul__, __div__, etc. to that metaclass, so that we could simplify the implementation of may metrics

For example

F2 = Precision * Recall * 2 / (Precision + Recall)
F2.__doc__ = """...."""

Then we would have a class F2 implemented very simple. This allows us to easily improve the coverage of metrics,

[vfdev-5]

What in my mind was, add __add__, __mul__, __div__, etc. to that metaclass, so that we could simplify the implementation of may metrics

@zasdfgbnm the implementation you had in mind is similar to what you proposed using MetricsLambda ?
If not could you please provide more details on the implementation of __add__, __mul__ etc

[zasdfgbnm]

@vfdev-5 It is a little bit similar, but for this the operands are classes like Precision, and the result are also class. The idea is, if A and B are classes for metrics, then A + B is another class for a new metric. Should be something like

class MetricMeta(type):

    def __add__(cls1, cls2):
        if "signature of __init__ of cls1 and cls2 does not match":
            raise "can not add cls1 and cls2"

        class Result(Metric):

            def __init__(self, *args, **kwargs):
                self.obj1 = cls1(*args, **kwargs)
                self.obj2 = cls2(*args, **kwargs)

            def reset(self):
                self.obj1.reset()
                self.obj2.reset()

            def update(self, output):
                self.obj1.update(output)
                self.obj2.update(output)

            def compute(self):
                return self.obj1.compute() + self.obj2.compute()

        return Result

[vfdev-5]

@zasdfgbnm I definitly like this approach more that with inceptions of MapLambda. So, now we have arithmetics + reset-update-compute logic that suggest to create a metaclass for metric. I would just make sure that reset-update-compute logic can't be done differently than you proposed in #336

[zasdfgbnm]

@vfdev-5 Now I think metaclass seems not to be a very good approach of implementing reset-update-compute logic, because wrapping of reset, update and compute happens very early for metaclass approach. If in __init__ the self.update, etc. is replaced (which is not very uncommon) like in https://github.com/pytorch/ignite/blob/8db9d47dd3e5d6895d2a8751358ebc81b3e7b880/ignite/metrics/running_average.py#L50
then this no longer work.

Besides, metaclass is like a supreme leader that governs the creation of all subclasses to enforce this check to it. Whenever the user want to create a metric that has well defined value when no data feed in, the way to do it becomes very tricky. Reset-update-compute logic is not a very strong requirement that we have to enforce it that much.

The decorator approach might be a good approach. Another alternative is, in class Metric, define a method enable_reset_update_compute_check, which provide a mechanism for metric writers to enable this check by calling it in, e.g. __init__.

[vfdev-5]

@zasdfgbnm I see your point. Need to think more on it