converter for SoftmaxCrossEntropy #107

ir5 · 2019-02-26T04:08:53Z

I implemented converter for SoftmaxCrossEntropy. Since I used OneHot node in this implementation, onnx==1.4.1 is required. (Lower version may suffice, but I'm not sure.)
Testing using onnxruntime is not done because onnxruntime currently does not support OneHot node.
To check the validity of the implementation, I temporally wrote the following script to generate a test case.

import chainer
import numpy as np

import onnx_chainer

class Model(chainer.Chain):
    def __init__(self):
        super(Model, self).__init__()

    def __call__(self, x, t):
        return chainer.functions.softmax_cross_entropy(x, t)

model = Model()
x = np.random.uniform(size=(3, 50)).astype('f')
t = np.random.randint(size=3, low=0, high=50).astype(np.int32)
x = chainer.as_variable(x)
t = chainer.as_variable(t)
onnx_chainer.export_testcase(model, (x, t), 'softmax')

Then, I ran run_onnx in chainer-compiler. The result was "OK!", so I suppose that the converter works correctly.

./build/tools/run_onnx --test </path/to/softmax> --trace -d cuda

…_cross_entropy

shinh

Defer to tanaka-san

shinh · 2019-02-26T06:16:43Z

onnx_chainer/functions/loss.py

+            np.array([x_var.shape[1]], dtype=np.int32))))
+    nodes.append(helper.make_node(
+        'Constant', [], [zeroone], value=from_array(
+            np.array([0, 1], dtype='f'))))


I think you can use dtype=x_var.dtype?

shinh · 2019-02-26T06:18:17Z

tests/functions_tests/test_loss.py

@@ -0,0 +1,38 @@
+# Currently, the test for SoftmaxCrossEntropy is disabled since onnxruntime


Could you create a bug and add its URL here?

Sorry, what do you mean "create a bug and add its URL here"?

Sorry, I meant an issue: https://github.com/chainer/onnx-chainer/issues/new

Understood.

disktnk · 2019-02-26T08:40:34Z

onnx_chainer/functions/loss.py

+from onnx.numpy_helper import from_array
+
+from onnx_chainer.onnx_helper import gensym
+from onnx_chainer import onnx_helper


please change import order?

onnx_chainer/functions/loss.py:6:1: H306 imports not in alphabetical order (onnx_chainer.onnx_helper.gensym, onnx_chainer.onnx_helper)

Coding style guide of Chainer follows "OpenStack Style Guidelines", but currently hacking test is not set in CI because of dependency conflict, sorry for inconvenient.

I know that hacking was used in Chainer for long time, but it seem now hacking is not in requirements in the coding style guide of Chainer? https://github.com/chainer/chainer/blob/master/setup.py#L34-L39
Without hacking, both flake8 and autopep8 do not output format error.

Exactly, Chainer remove hacking from requirement, so they fix import order or other to fit OpenStack Style Guildelines manually like this chainer/chainer#6128

Understood. Thank for for clarifying the rule.

disktnk · 2019-02-26T08:47:56Z

onnx_chainer/functions/loss.py

+        func, opset_version, input_names,
+        num_outputs, parameters):
+    # obtain input variable
+    x_var, t_var = func.get_retained_inputs()


On Chainer 5.2.0=current stable version, SoftmaxCrossEntropy is not supported get_retained_inputs method because F.SoftmaxCrossEntropy is not a subclass of FunctionNode, is subclass of Function. ONNX-chainer don't have to support old version, I think, but I would like to support at least stable version.

Thank you for noting important point. Unfortunately, it seems like there's no easy way to retrieve input variables (x and t) just from the interface of Function... https://github.com/chainer/chainer/blob/v5.2.0/chainer/functions/loss/softmax_cross_entropy.py#L165
Since this converter is an experimental feature, I may want to a version requirements for this feature to keep the implementation simple. I may put an explicit error message when non-latest version Chainer is used.

disktnk · 2019-02-26T08:50:35Z

onnx_chainer/functions/loss.py

+    x_var, t_var = func.get_retained_inputs()
+    if len(x_var.shape) != 2:
+        raise NotImplementedError(
+            'onnx-chainer currently handles SoftmaxCrossEntropy only when '


Currently error and warning messages are unified "ONNX-C hainer", could you follow it? L20, L25 is same

disktnk · 2019-02-26T09:01:25Z

onnx_chainer/functions/loss.py

+    depth = gensym()
+    zeroone = gensym()
+
+    nodes.append(helper.make_node(


Is there any special reason not to use onnx_helper? ref https://github.com/chainer/onnx-chainer/blob/master/onnx_chainer/functions/array.py#L53

I thinks the below code can work, not to call gensym function, but I'd like to know if you have reasons.

nodes.append(onnx_helper.make_node('LogSoftmax', [x], 1)) y_log = nodes[-1].output[0]

In this case, there are lots of intermediate values. Probably this is a personal preference, but I liked current style because this makes the inputs and outputs of a node more understandable. But if gensym is not supposed to be called from outside the helper module, then I would change the this part.

I agree nodes[-1] is not easy to read. One idea would be defining a helper function and use it like

def add_node(*args, **kwargs): node = onnx_helper.make_node(*args, **kwargs) nodes.append(node) return node.output[0] y_log = add_node(LogSoftmax, [x], 1)

However, I think it's better to more this kind of helper function to onnx_helper. Actually, I was planning to do this kind of refactoring by myself. So, how about keeping the code as is in this PR. We will see if I can make it better :)

I agree that it would be more helpful if some better function is implemented in the helper module. It sounds good to keep the code as is seems and try to improve later.

helper function looks nice, so approve current implementation, thx

disktnk · 2019-02-26T09:02:03Z

tests/functions_tests/test_loss.py

+# @testing.parameterize(
+#     {'in_shape': (3, 5)},
+# )
+# class TestSoftmaxCrossEntropy(unittest.TestCase):


I prefer to use @unittest.skip("OneHot operator is not supported on test runtime") decorator than comment out.

ir5 · 2019-02-27T03:32:41Z

I fixed the code except for gensym part.

Mitsuru Kusumoto added 5 commits February 25, 2019 12:06

(Not tested) add converter for SoftmaxCrossEntropy

cf36f15

Merge branch 'master' of github.com:chainer/onnx-chainer into softmax…

2deb267

…_cross_entropy

Update the I/F of convert function

a0f9c1e

Add test for SoftmaxCrossEntropy (comment-out)

b4605e7

Generic implementation

0682ca2

shinh reviewed Feb 26, 2019

View reviewed changes

ir5 mentioned this pull request Feb 26, 2019

OneHot is not implemented in onnxruntime==0.2.1 #108

Closed

dtype

4fc88c8

disktnk self-requested a review February 26, 2019 07:14

disktnk added this to the 1.3.3 milestone Feb 26, 2019

disktnk requested changes Feb 26, 2019

View reviewed changes

Mitsuru Kusumoto added 4 commits February 27, 2019 12:02

import order

fb5e3ad

ONNX-Chainer

db1d42f

skip test

7531850

explicit error message

75f7904

disktnk approved these changes Feb 27, 2019

View reviewed changes

disktnk merged commit 848a074 into chainer:master Feb 27, 2019

		@@ -0,0 +1,38 @@
		# Currently, the test for SoftmaxCrossEntropy is disabled since onnxruntime

converter for SoftmaxCrossEntropy #107

converter for SoftmaxCrossEntropy #107

Uh oh!

Conversation

ir5 commented Feb 26, 2019

Uh oh!

shinh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

disktnk Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ir5 commented Feb 27, 2019

Uh oh!

Uh oh!

disktnk Feb 26, 2019 •

edited

Loading