Revamped test cases for Gemm by JamesAllingham · Pull Request #2060 · onnx/onnx

JamesAllingham · 2019-05-28T20:10:44Z

Hi all,

I'm currently working on an ONNX importer for the neural network's framework in Wolfram Mathematica. While developing I've found it useful to add more test cases to make sure that I'm following the ONNX spec properly, so I figured I should contribute them back upstream so that others can also use them. I'll definitely be adding more in the near future, I just wanted to get my hands dirty with this to make sure I'm doing things correctly. So assuming that these changes are welcome I'll be adding more soon. :)

This is my first time contributing to the project, so please do let me know if I'm doing anything wrong! After adding the test cases, I re-installed ONNX, ran the test suite, as well as the type checker, and didn't see any issues. I also made linted the code to make sure it was all looking good.

Here is what the PR changes:

Increases the number of tests for Gemm from 2 to 8.
The tests now each test a specific attribute or input case.
Adds test data.
Updates the operators doc.
Updates the test coverage.

Regarding point 2, I wanted to make it easier for myself, and other developers, to incrementally add support for the attributes and other features of the Gemm operator. By splitting the tests up so that each one only covers a particular attribute or input case I think this goal is accomplished. It also makes it immediately clear what has broken when a test case regression occurs.

One thing I was a little worried about when making this change was that it would not be allowed to remove existing test cases. If this is the case I'll add the old cases back -- I simply removed them because they were a bit redundant. I also imagine there might be some feedback about how I've named the test cases.

* Increased number of tests from 2 to 8. * The tests each test a specific attribute or input case. * Generated test data. * Updated the operators doc. * Updated the test coverage.

CLAassistant · 2019-05-28T20:10:53Z

All committers have signed the CLA.

linkerzhang · 2019-05-30T00:18:06Z

@JamesAllingham welcome and thank you for the contribution to ONNX. It's ok to remove some test cases as long as the scenarios/cases are also covered by new ones.

Please fix the CI failures:

+flake8
./onnx/backend/test/case/node/gemm.py:24:24: E231 missing whitespace after ','
./onnx/backend/test/case/node/gemm.py:72:29: E261 at least two spaces before inline comment
./onnx/backend/test/case/node/gemm.py:81:24: E231 missing whitespace after ','
./onnx/backend/test/case/node/gemm.py:87:29: E261 at least two spaces before inline comment
./onnx/backend/test/case/node/gemm.py:96:24: E231 missing whitespace after ','
./onnx/backend/test/case/node/gemm.py:111:24: E231 missing whitespace after ','

JamesAllingham · 2019-05-30T07:29:17Z

@linkerzhang Thanks for the welcome. I'm very keen to be getting involved with this project. And thanks for the feedback.

What my test cases don't cover (that the old ones did) is whether the transA, transB, alpha and beta parameters work as expected when used together. However, that seems like a bit of an edge case, and to properly test all interactions like that for every op would require a huge number of tests. That said, I suppose there is nothing wrong with having a few of these sorts of test cases. Let me know what you think.

…emm tests.

JamesAllingham · 2019-06-17T14:32:15Z

Any feedback on this? Would be great to get this closed so that I can go ahead with some more PRs (would be nice to know I'm doing things correctly before doing more 😃)

JamesAllingham · 2019-07-10T14:48:13Z

Hey @linkerzhang any feedback on this? I also have another PR which could use some love. I have quite a few more tests I'd like to submit PRs for but I really would like to get some feedback on what I'm doing before submitting!

postrational

This looks good to me. I think we should merge it.
@JamesAllingham, could you update your branch?

JamesAllingham · 2019-09-02T13:00:27Z

@postrational I've updated the branch. Let me know if I need to do anything else to get this merged.

wschin · 2019-09-16T05:09:27Z

+a = np.random.ranf([3, 5]).astype(np.float32)
+b = np.random.ranf([5, 4]).astype(np.float32)
+c = np.zeros([1, 4]).astype(np.float32)
+y = 0.5 * np.dot(a, b) + c


[nit] This is good enough for tests. On the other hand, it'd be nice if you can create a reference implementation of Gemm using numpy and use it in all tests. For example,

y = your_gemm_reference_impl(a, b, c, ...)

I agree, it will be good to have a reference implementation, I'll add one. 👍

wschin · 2019-09-16T05:11:53Z

+b = np.random.ranf([5, 4]).astype(np.float32)
+c = np.zeros([1, 4]).astype(np.float32)
+y = np.dot(a, b) + c
+expect(node, inputs=[a, b, c], outputs=[y],


No bias? Do you mean that c will be removed?

What I meant was that the bias term has no impact on the result. I think this is a useful test case because in many frameworks the bias term of the fully-connected layer is optional with a default of 0. I'll rename the test to make this more clear.

With that in mind, do you think it is worth changing the spec for Gemm so that the bias is optional and has a default of 0? I'm happy to do so, and I don't think it is too much work or too big a change. (Then I can add a test which legitimately has no bias term).

It totally makes sense to me. Could we do it in another PR? I will try to get it reviewed (and then hopefully merged) before the deadline (9/19 is the targeted release date)? I remember that I had to insert zero initializers when converting cases without bais. The case with C=0 is also a common case in practices, so we should have an one-to-one mapping in the IR.

Sure, I'll make another PR for it. I'm afraid it will have to be about 11 hours from now though!

wschin · 2019-09-16T05:17:22Z

+)
+a = np.random.ranf([2, 3]).astype(np.float32)
+b = np.random.ranf([3, 4]).astype(np.float32)
+c = np.random.ranf([1]).astype(np.float32)


[1] is not a scalar. It's a 1-element vector. Maybe we can use np.array(1) whose shape is (). As mentioned in my comment above, it'd even nicer if we have a reference implementation.

You are totally correct. I've fixed this. And I am going to do a reference implementation.

wschin · 2019-09-16T05:19:20Z

    'Gemm',
    inputs=['a', 'b', 'c'],
    outputs=['y'],
-    alpha=0.5,


May we keep this one? It represents a case where all attributes are non-default.

Agreed and done 👍

(I renamed this test, it is now called 'test_gemm_all_attributes')

wschin

This PR looks very good. Could you please take a look at my minor comments and see if we can address them? Thank you! To me more clear, I think this PR is almost ready to be merged.

1. Differentiated between sclar and single element vector biases. 2. Changed the name of 'test_gemm_default_no_bias' to 'test_gemm_default_zero_bias' to be more clear. 3. Added back the test case for specifying all attributes as non-default.

JamesAllingham · 2019-09-17T08:27:11Z

This PR looks very good. Could you please take a look at my minor comments and see if we can address them? Thank you! To me more clear, I think this PR is almost ready to be merged.

Cool, I think I've addressed all of your comments, please let me know if I misunderstood anything. Also, let me know what you think about allowing the bias input ('C') to be optional with a default of 0.

Also added the mypy type annotation for the Gemm reference implementation.

… more_gemm_tests

wschin · 2019-09-17T20:30:25Z

-c = np.zeros([1, 4]).astype(np.float32)
+a = np.random.ranf([2, 3]).astype(np.float32)
+b = np.random.ranf([3, 4]).astype(np.float32)
+c = np.random.ranf(1).astype(np.float32)


This is still not a scalar.

>>> np.random.ranf(1).shape (1,)

As you already have a reference implementation, could we do

Suggested change

c = np.random.ranf(1).astype(np.float32)

c = 3.14 or c=np.array(3.14)

?

Whoops! Let me fix that!

Okay, I think I've fixed this for real now, sorry about that :)

wschin · 2019-09-17T20:35:24Z

+    A = A if transA == 0 else A.T
+    B = B if transB == 0 else B.T
+
+    Y = alpha * np.dot(A, B) + beta * C


wschin

Overall looks good and it improves the document considerablely. Just last few comments to address.

JamesAllingham · 2019-09-17T21:37:57Z

Overall looks good and it improves the document considerablely. Just last few comments to address.

One unfortunate thing about the documentation is that the reference implementation is not shown in Operators.md, I don't suppose there is an easy fix for this?

* Revamped test cases for Gemm * Increased number of tests from 2 to 8. * The tests each test a specific attribute or input case. * Generated test data. * Updated the operators doc. * Updated the test coverage. * Fixed linting errors causing CI failures * Updated the TestCoverage and Operators docs for the newly re-linted Gemm tests. * Tweaks to Gemm test cases: 1. Differentiated between sclar and single element vector biases. 2. Changed the name of 'test_gemm_default_no_bias' to 'test_gemm_default_zero_bias' to be more clear. 3. Added back the test case for specifying all attributes as non-default. * Added a reference implementation for the Gemm op * Fixed some mistakes with the types of transA and transB. Also added the mypy type annotation for the Gemm reference implementation. * The Gemm scalar bias test actually adds a scalar bias now

Revamped test cases for Gemm

0ff4d82

* Increased number of tests from 2 to 8. * The tests each test a specific attribute or input case. * Generated test data. * Updated the operators doc. * Updated the test coverage.

JamesAllingham requested a review from a team as a code owner May 28, 2019 20:10

Fixed linting errors causing CI failures

8782c0d

Updated the TestCoverage and Operators docs for the newly re-linted G…

ea2a8ec

…emm tests.

prasanthpul added the topic: operator Issues related to ONNX operators label Jun 10, 2019

Merge branch 'master' into more_gemm_tests

81fa3f8

postrational approved these changes Aug 22, 2019

View reviewed changes

Fixed conflicts in Operators.md

d75639a

prasanthpul added this to the 1.6 milestone Sep 9, 2019

Merge branch 'master' into more_gemm_tests

3a662bb

wschin reviewed Sep 16, 2019

View reviewed changes

wschin requested changes Sep 16, 2019

View reviewed changes

wschin and others added 5 commits September 15, 2019 22:21

Merge branch 'master' into more_gemm_tests

61a6562

Merge branch 'master' into more_gemm_tests

cbf18b5

Tweaks to Gemm test cases:

773721a

1. Differentiated between sclar and single element vector biases. 2. Changed the name of 'test_gemm_default_no_bias' to 'test_gemm_default_zero_bias' to be more clear. 3. Added back the test case for specifying all attributes as non-default.

Added a reference implementation for the Gemm op

ef52cad

Merge branch 'master' into more_gemm_tests

135e9f6

JamesAllingham added 2 commits September 17, 2019 11:36

Fixed some mistakes with the types of transA and transB.

90a3cdc

Also added the mypy type annotation for the Gemm reference implementation.

Merge branch 'more_gemm_tests' of github.com:JamesAllingham/onnx into…

99761ba

… more_gemm_tests

wschin self-requested a review September 17, 2019 20:18

Merge branch 'master' into more_gemm_tests

06e39cd

wschin reviewed Sep 17, 2019

View reviewed changes

wschin approved these changes Sep 17, 2019

View reviewed changes

The Gemm scalar bias test actually adds a scalar bias now

076cf51

JamesAllingham force-pushed the more_gemm_tests branch from 6d91ea2 to 076cf51 Compare September 17, 2019 21:20

wschin merged commit d0c697b into onnx:master Sep 17, 2019

JamesAllingham mentioned this pull request Sep 18, 2019

Gemm optional bias #2330

Merged

	c = np.random.ranf(1).astype(np.float32)
	c = 3.14 or c=np.array(3.14)

Conversation

JamesAllingham commented May 28, 2019

Uh oh!

CLAassistant commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linkerzhang commented May 30, 2019

Uh oh!

JamesAllingham commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesAllingham commented Jun 17, 2019

Uh oh!

JamesAllingham commented Jul 10, 2019

Uh oh!

postrational left a comment

Choose a reason for hiding this comment

Uh oh!

JamesAllingham commented Sep 2, 2019

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!

wschin Sep 17, 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!

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!

wschin left a comment • 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!

JamesAllingham commented Sep 17, 2019

Uh oh!

wschin Sep 17, 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!

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!

wschin left a comment

Choose a reason for hiding this comment

Uh oh!

JamesAllingham commented Sep 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CLAassistant commented May 28, 2019 •

edited

Loading

JamesAllingham commented May 30, 2019 •

edited

Loading

wschin Sep 17, 2019 •

edited

Loading

wschin left a comment •

edited

Loading

wschin Sep 17, 2019 •

edited

Loading