Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Gemm optional bias #2330

Merged
merged 8 commits into from Sep 18, 2019
Merged

Gemm optional bias #2330

merged 8 commits into from Sep 18, 2019

Conversation

JamesAllingham
Copy link
Contributor

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
Contributor

wschin commented Sep 18, 2019 •
edited

+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
Contributor

spandantiwari commented Sep 18, 2019 •
edited

@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
docs/Operators.md Outdated Show resolved Hide resolved
wschin
wschin reviewed Sep 18, 2019
View reviewed changes
docs/Operators.md Outdated Show resolved Hide resolved
wschin
wschin reviewed Sep 18, 2019
View reviewed changes
onnx/defs/math/defs.cc Outdated Show resolved Hide resolved
@wschin
Copy link
Contributor

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
Contributor

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
Contributor Author

@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
Contributor

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 operator Issues related to ONNX operators label Sep 18, 2019
@JamesAllingham
Copy link
Contributor Author

@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
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.

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
Contributor

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
Contributor Author

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

@spandantiwari spandantiwari spandantiwari approved these changes

@houseroad houseroad Awaiting requested review from houseroad

@gramalingam gramalingam Awaiting requested review from gramalingam

Assignees
No one assigned
Labels
operator Issues related to ONNX operators
Projects
None yet
Milestone
1.6
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JamesAllingham @wschin @spandantiwari @prasanthpul