Misleading calculation of mxnet.metric.Accuracy

[Ishitori]

Description

When passes "preds" and "labels" arguments of the same shape to mxnet.metric.Accuracy.update() method, they get silently converted to ints and compared element-wise. This can lead to a weird behavior when "labels" actually are in one-hot encoded format. See the case on StackOverflow: https://stackoverflow.com/questions/45798125/why-mxnet-is-reporting-the-incorrect-validation-accuracy/48471241

I recommend add a warning message if the conversion is happening from floats to int. Removing the conversion is also possible, but it is a breaking change.

Environment info (Required)

----------Python Info----------
Version      : 3.6.2
Compiler     : GCC 4.2.1 Compatible Apple LLVM 8.1.0 (clang-802.0.42)
Build        : ('default', 'Jul 17 2017 16:44:45')
Arch         : ('64bit', '')
------------Pip Info-----------
Version      : 9.0.1
Directory    : /usr/local/lib/python3.6/site-packages/pip
----------MXNet Info-----------
Version      : 0.11.0
Directory    : /usr/local/lib/python3.6/site-packages/mxnet
Commit Hash   : 53274b4a2b0d73f3fbdb10cfb5f9ed0c8263fda7
----------System Info----------
Platform     : Darwin-16.7.0-x86_64-i386-64bit
system       : Darwin
node         : <removed>
release      : 16.7.0
version      : Darwin Kernel Version 16.7.0: Thu Jan 11 22:59:40 PST 2018; root:xnu-3789.73.8~1/RELEASE_X86_64
----------Hardware Info----------
machine      : x86_64
processor    : i386
b'machdep.cpu.extfeatures: SYSCALL XD 1GBPAGE EM64T LAHF LZCNT PREFETCHW RDTSCP TSCI'
b'machdep.cpu.leaf7_features: SMEP ERMS RDWRFSGS TSC_THREAD_OFFSET BMI1 HLE AVX2 BMI2 INVPCID RTM SMAP RDSEED ADX IPT SGX FPU_CSDS MPX CLFSOPT'
b'machdep.cpu.features: FPU VME DE PSE TSC MSR PAE MCE CX8 APIC SEP MTRR PGE MCA CMOV PAT PSE36 CLFSH DS ACPI MMX FXSR SSE SSE2 SS HTT TM PBE SSE3 PCLMULQDQ DTES64 MON DSCPL VMX SMX EST TM2 SSSE3 FMA CX16 TPR PDCM SSE4.1 SSE4.2 x2APIC MOVBE POPCNT AES PCID XSAVE OSXSAVE SEGLIM64 TSCTMR AVX1.0 RDRAND F16C'
b'machdep.cpu.brand_string: Intel(R) Core(TM) i7-7660U CPU @ 2.50GHz'
----------Network Test----------
Setting timeout: 10
Timing for MXNet: https://github.com/apache/incubator-mxnet, DNS: 0.0030 sec, LOAD: 0.6859 sec.
Timing for Gluon Tutorial(en): http://gluon.mxnet.io, DNS: 0.0009 sec, LOAD: 0.1113 sec.
Timing for Gluon Tutorial(cn): https://zh.gluon.ai, DNS: 0.0015 sec, LOAD: 0.2410 sec.
Timing for FashionMNIST: https://apache-mxnet.s3-accelerate.dualstack.amazonaws.com/gluon/dataset/fashion-mnist/train-labels-idx1-ubyte.gz, DNS: 0.0007 sec, LOAD: 0.2651 sec.
Timing for PYPI: https://pypi.python.org/pypi/pip, DNS: 0.0008 sec, LOAD: 0.2126 sec.
Timing for Conda: https://repo.continuum.io/pkgs/free/, DNS: 0.0006 sec, LOAD: 0.1419 sec.

Error Message:

There is no visible error message

Minimum reproducible example

import mxnet as mx
predicts = mx.nd.array([[1.29206967e-09,   3.40120096e-05,   2.23299547e-12,   3.98692492e-07,
    1.21151755e-10,   2.59370694e-08,   1.95488334e-02,   1.13474562e-05,
    9.80405331e-01,   3.51648767e-12]])
labels = mx.nd.array([[0, 0, 0, 0, 0, 0, 0, 0, 1, 0]])
acc = mx.metric.Accuracy()
acc.update(preds=predicts, labels=labels)
print(acc.get())

which gives us:

('accuracy', 0.90000000000000002)

[szha]

The accuracy class is not intended to be used for one-hot labels. I'm clarifying that in the api doc in the above PR. In terms of feature, I think it would be fairly easy for the user to do argmax on one-hot label and then feed that into metric.

[Ishitori]

Agree, it is an incorrect usage of a class. Yet, silent casting is happening, which makes it hard to spot a problem. Shouldn't we try to make mxnet prone to mistakes like that?

[szha]

Is there a case where casting would be a problem under proper usage?

[Ishitori]

It shouldn't be a problem under a correct usage, but there is a way to use it incorrectly and system doesn't report that. If we could report it, that would increase an ease of use.

Anyway, it is up to you guys to decide if it is worth fixing.

[szha]

Thanks for the feedback.

[ruro]

Why was this issue closed?

The behavior of Accuracy.update is still wrong for one-hot labels. It still doesn't raise any error/warning and just silently gives the wrong values. The current documentation for the Accuracy class doesn't mention, whether preds/labels can be one-hots or indices.

The doc string for the update method does mention, that labels should contain "class indices as values", but the way it's worded doesn't strongly imply, that it can't be a one-hot vector. Given, that preds does accept a probability vector, it's a reasonable assumption, that labels also would.

Also, I think, the docstring for the update method doesn't actually get rendered into the current web docs. At least I can't find it anywhere.

I don't see, what is the "use case" for the current behavior. For example, if preds.shape == labels.shape == (32, 10), the current implementation would just truncate both preds and labels to integers and compare them for equality. Why would this be useful?

According to the current docs, passing labels of shape (32, 10) should be invalid anyway, because an array of shape (10,) is not a "class index as a value, one per sample". So even if somebody actually relies on the current way, they are using undocumented, undefined behavior.

[leezu]

@szha