Skip to content

Gemm optional bias#2330

Merged
wschin merged 8 commits intoonnx:masterfrom
JamesAllingham:gemmOptionalBias
Sep 18, 2019
Merged

Gemm optional bias#2330
wschin merged 8 commits intoonnx:masterfrom
JamesAllingham:gemmOptionalBias

Conversation

@JamesAllingham
Copy link
Copy Markdown
Contributor

@JamesAllingham JamesAllingham commented Sep 18, 2019

As discussed (briefly) in #2060 it would be useful for the bias term (the 'C' input) of the 'Gemm' operation to be optional. This PR adds this functionality.

Specifically, if the bias is not specified it is assumed to be 0. This is a common behaviour among deep learning frameworks.

This PR:

  1. Updates the definition (in math/defs.cc) to make the 'C' input optional. The version for the Gemm operator is bumped to 11, and the old definition is moved to math/old.cc. Similarly, operator_sets.h is updated to reflect the new version number.

  2. Updates the gemm_reference_implementation to allow for 'C' being optional.

  3. Adds a test case for no bias term.

@JamesAllingham
Made the 'C' input of Gemm (the bias term) optional.
5206884 
If missing it defaults to 0.

Also added a test case for no bias.

Updated the Gemm op to version 11.
@JamesAllingham
Fixed a typo!
fbb54d6
@JamesAllingham JamesAllingham requested a review from a team as a code owner September 18, 2019 09:38
@wschin
Copy link
Copy Markdown
Collaborator

wschin commented Sep 18, 2019 •
edited
Loading

+some people from SIG operator @ebarsoum, @spandantiwari, @gramalingam for visibility. I personally consider this change reasonable for some reasons below.

  1. We have MatMul and Add but still introduced Gemm for this opportunity of further computation optimization. Because 2-D matrix multiplication is even more fundamental than Gemm, I feel it's reasonable to allow it in Gemm (a symbol stands for ultimate optimization).
  2. Most frameworks can easily implement it given their existing Gemm code.
  3. From user's perspective, it's sometimes annoying to use Gemm because we need to create zero initializer only for matching the spec.

To be fair, I also should note that this change is not going to increase the expressiveness of ONNX.

@wschin wschin mentioned this pull request Sep 18, 2019
Closed
@spandantiwari
Copy link
Copy Markdown
Contributor

spandantiwari commented Sep 18, 2019 •
edited
Loading

@wschin @JamesAllingham Just curious if this is being tracked for 1.6 release. If not, then we may have to update the opset version number in the PR from 11 to 12.

wschin
wschin reviewed Sep 18, 2019
View reviewed changes
Comment thread docs/Operators.md Outdated
wschin
wschin reviewed Sep 18, 2019
View reviewed changes
Comment thread docs/Operators.md Outdated
wschin
wschin reviewed Sep 18, 2019
View reviewed changes
Comment thread onnx/backend/test/case/node/gemm.py
wschin
wschin reviewed Sep 18, 2019
View reviewed changes
Comment thread onnx/defs/math/defs.cc Outdated
@wschin
Copy link
Copy Markdown
Collaborator

wschin commented Sep 18, 2019

@wschin @JamesAllingham Just curious if this is being tracked for 1.6 release. If not, then we may have to update the opset version number in the PR from 11 to 12.

It's a nice to have thing in 1.6. If we can't make it on time, we can bump the opset version if you want.

@prasanthpul prasanthpul added this to the 1.6 milestone Sep 18, 2019
@wschin
Copy link
Copy Markdown
Collaborator

wschin commented Sep 18, 2019

@JamesAllingham, we need a shape inference test as well.

@JamesAllingham JamesAllingham requested a review from a team as a code owner September 18, 2019 20:44
@JamesAllingham
Copy link
Copy Markdown
Contributor Author

JamesAllingham commented Sep 18, 2019

@JamesAllingham, we need a shape inference test as well.

I've added a test but I wasn't 100% sure what you wanted here so let me know if you wanted something else.

@wschin
Copy link
Copy Markdown
Collaborator

wschin commented Sep 18, 2019

@JamesAllingham, we need a shape inference test as well.

I've added a test but I wasn't 100% sure what you wanted here so let me know if you wanted something else.

Ah, my bad. I missed the last file.

@wschin wschin added the topic: operator Issues related to ONNX operators label Sep 18, 2019
@JamesAllingham
Copy link
Copy Markdown
Contributor Author

JamesAllingham commented Sep 18, 2019

@JamesAllingham, we need a shape inference test as well.
I've added a test but I wasn't 100% sure what you wanted here so let me know if you wanted something else.

Ah, my bad. I missed the last file.

No, I only just added it, you didn't miss anything!

spandantiwari
spandantiwari approved these changes Sep 18, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@spandantiwari spandantiwari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Comment thread onnx/defs/math/old.cc
auto& first_input_shape = getInputShape(ctx, 0);
auto& second_input_shape = getInputShape(ctx, 1);
if (first_input_shape.dim_size() != 2)
fail_shape_inference("First input does not have rank 2");
Copy link
Copy Markdown
Contributor

@spandantiwari spandantiwari Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Minor point about code style - consider adding braces even for single line scopes to be consistent with coding style in this file.

Copy link
Copy Markdown
Contributor Author

@JamesAllingham JamesAllingham Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do 👍

@wschin wschin merged commit 23bb6ea into onnx:master Sep 18, 2019
kevinch-nv pushed a commit that referenced this pull request Sep 23, 2019
@wschin @kevinch-nv
Sync master and 1.6.0 release for adding fixes and improvements (#2344)
7d847cb 
* Gemm optional bias (#2330)

* Made the 'C' input of Gemm (the bias term) optional.

If missing it defaults to 0.

Also added a test case for no bias.

Updated the Gemm op to version 11.

* Fixed a typo!

* Small tweaks to the Gemm docs.

* Added a shape inference test for Gemm with no bias

* Tweaked coding style slightly by adding braces to single line scopes.

* Fix some backend tests  (#2335)

* Fix some node tests

* PR comments and docs

* Update Changelog.md

* Update gen_doc script to validate proto3 files (#2122)

* Update gen_doc script to validate proto3 files

* Update CMakeLists.txt

* Update pybind (#2340)

* Fix node test case model for Gemm scalar bias case (#2342)

* Fix some node tests

* PR comments and docs

* Update Changelog.md

* Fix gemm scalar node test

* Clarify behavior in ConvTranspose (#2343)

* Fix the wrong behavior in ConvTranspose

* Address comments
jcwchen pushed a commit to jcwchen/onnx that referenced this pull request Sep 23, 2020
@JamesAllingham @wschin
Gemm optional bias (onnx#2330)
2cc33e4 
* Made the 'C' input of Gemm (the bias term) optional.

If missing it defaults to 0.

Also added a test case for no bias.

Updated the Gemm op to version 11.

* Fixed a typo!

* Small tweaks to the Gemm docs.

* Added a shape inference test for Gemm with no bias

* Tweaked coding style slightly by adding braces to single line scopes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wschin wschin wschin approved these changes

@houseroad houseroad Awaiting requested review from houseroad

@gramalingam gramalingam Awaiting requested review from gramalingam

+1 more reviewer

@spandantiwari spandantiwari spandantiwari approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

topic: operator Issues related to ONNX operators

Projects

None yet

Milestone

1.6

Development

Successfully merging this pull request may close these issues.

4 participants

@JamesAllingham @wschin @spandantiwari @prasanthpul