Add support for nested metric values. (#959)

[failable]

Related to #959

Description:

This commit add support for nested metric values. If Metric.compute returns a dict, its values will be flattened into engine.state.metrics in Metric.completed.

Note that, in this case the value types of the returned dict is limited(i.e. Dict[Any, non number type] is not allowed), while the original allowed type of Metric.compute is Any. We can also allow other types of dict values, but this may be lead unclear behaviors to users when they try to return (different kinds of )dicts.

Check list:

• New tests are added (if a new feature is added)
• New doc strings: description and/or example code are in RST format
• Documentation is updated (if required)

[sdesrozis]

I don’t know if an exception should be raised. Metrics are very generics, it can work with ˋAny`. Metrics associated to loggers should use restricted types.

@vfdev-5 ?

[failable]

I agree metrics are generics. Maybe remove type checking here and move to Logger later when necessary?

[sdesrozis]

Ok, let’s try this! Just flat dict here (with compatible existing tests)

[failable]

I just found one more issue, if Any type is allowed, shall we deep flatten a dict in this case? Or shall me only care about dict contains all number values?

[vfdev-5]

IMO, we should not check the output type and raise an error and just keep previous behaviour.
Previously, we had a part of code that applied .item() on torch scalars. Maybe just factorize this code can be enough.

[sdesrozis]

I agree. At the moment, we should flat only the result of metrics. IMHO, what is embedded in dict is related to the metric itself.

We would notice in doc that generic result type ˋAny` is a generic purpose but interoperability with others handlers / features implies some restrictions : scalars (number, tensor) and dict of scalars.

What do you think ?

And thank you again for your help 🙏🏻

[vfdev-5]

Suggested change

	if isinstance(result, dict):
	if isinstance(result, Mapping):

[vfdev-5]

IMO, idea of the PR is to unnest one level of mappings (e.g. dict) as @Isolet's use-case. Thoughts ?

[failable]

My original thought is single enough just to flatten a level one dict of metrics. But as metrics is so generic, now I think maybe we should also not to make level one dict anything special? Before I opened the issue, I used the following snippet to get my goal.

class MultipleMetricsOutputHandler(OutputHandler):

  def _setup_output_metrics(self, engine):
      metrics = {}
      if self.metric_names is not None:
          if (isinstance(self.metric_names, str)
                  and self.metric_names == 'all'):
              metrics = flatten_dict(engine.state.metrics)
          else:
              for name in self.metric_names:
                  if name not in engine.state.metrics:
                      warnings.warn('Provided metric name \'{}\' is missing '
                                    'in engine\'s state metrics: {}'.format(
                                        name,
                                        list(engine.state.metrics.keys())))
                      continue
                  metrics[name] = engine.state.metrics[name]

      if self.output_transform is not None:
          output_dict = self.output_transform(engine.state.output)

          if not isinstance(output_dict, dict):
              output_dict = {'output': output_dict}

          metrics.update(
              {name: value
               for name, value in output_dict.items()})

      return metrics

How about keeping anything of Metric unchanged? Is there a better place to achieve my goal? I'm not familiar enough with ignite so I can not get an answer now ...

---

But still, I found some functions like save_best_model_by_val_score and add_early_stopping_by_val_score access engine.state.metrics value directly, flattening may still has its place...Hooks can help?

[vfdev-5]

@Isolet I think one level flattening is a good level of simplifying things and keeping Metric rather generic. For other non-trivial cases, use should write its adapters.

[vfdev-5]

Let's remove those type limitations:

Any: the actual quantity of interest. However, if a :class:`dict` is returned, it will flattened 
into `engine.state.metrics` when :func:`~ignite.metrics.Metric.completed` is called.

[failable]

Done!

[sdesrozis]

LGTM but CI quite unstable...

[sdesrozis]

LGTM! Thank you very much !!

[failable]

Thanks!