Skip to content
This repository has been archived by the owner on May 31, 2024. It is now read-only.

Add Constant operator #116

Merged
merged 6 commits into from
Sep 20, 2019
Merged

Add Constant operator #116

merged 6 commits into from
Sep 20, 2019

Conversation

mattn
Copy link
Collaborator

@mattn mattn commented Aug 28, 2019

No description provided.

@owulveryck
Copy link
Owner

Tanks for the PR.

The test fails because the test constant creates an empty graph which is not handled by Gorgonia (obviously).

The source of the failing test is:


which comes from this test https://github.com/onnx/onnx/tree/master/onnx/backend/test/data/node/test_constant

The problem is that the graph is composed of a single node which does not have any input. It is normal for a constant to not have any input, but an operator should. My opinion is that a constant should not be considered as an operator. the ONNX team seemed to disagree :D

https://github.com/onnx/onnx/blob/master/docs/Operators.md#constant

I will investigate to see if it is something that can be easily fixed.

@mattn
Copy link
Collaborator Author

mattn commented Aug 28, 2019

Probably, we can test ssd if onnx-go support constant and unsqueeze.

@owulveryck
Copy link
Owner

owulveryck commented Sep 17, 2019

The official onnx project gave a reply to the issue 2274 regarding the constant operator.

As expected, the IR will not change (indeed, It would have been a surprise that they change it for us). Therefore we may need to address this particular case.

Two other operators have the same behavior:

Thus, RandomUniform and RandomNormal are two other examples of an operator with no inputs.

By now, I have no idea how to fix this in an idiomatic way.
My only idea is to detect those operators within the decoding process (in the decodeProto method), and to turn them into initializers as mentioned in the original issue:

There are "optimizer" passes that convert uses of "Constant" to replace it with an Initializer (just like you mention).

WDYT?

@mattn
Copy link
Collaborator Author

mattn commented Sep 17, 2019

So do you think Constant should always return empty?

@owulveryck
Copy link
Owner

TBH I don't know anymore. I thought we could do a particular case in the decoder to handle the "Constant operator" and turn it into an initializer. The Gorgonia's ExprGraph would have been valid and composed of a single node. But this will not work for the other input-less operators.

Another option is to fake an input by creating a new node of type DataCarrier in onnx-go's *Model structure if we detect a node that has no input. Then we attach this input to the operator to turn it into something valid.

The resulting graph would look like:

   | NewInput with the |
   | Shape and type    |
   | of the Operator   |
   | `DataCarrier`     | 
           |
           |
           \/
   | Node Operation  |
   | OpType constant |

Then within Gorgonia, we create an operator that overwrites its input (see the godoc of the Op interface, the method should return 0).
This would not impact the performances and create a valid graph in all the cases.

I don't know if my explanation is clear enough, though. If it is not, I can try to quickly implement the RandomNormal operator as an illustration if you want. Let me know.

@owulveryck
Copy link
Owner

I have created issue #131 to follow the implementation as well as a branch.

@codecov
Copy link

codecov bot commented Sep 20, 2019

Codecov Report

Merging #116 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #116      +/-   ##
=========================================
+ Coverage   85.19%   85.2%   +0.01%     
=========================================
  Files         394     395       +1     
  Lines       10434   10445      +11     
=========================================
+ Hits         8889    8900      +11     
  Misses       1338    1338              
  Partials      207     207
Impacted Files Coverage Δ
backend/x/gorgonnx/constant.go 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d48630...90cb9f2. Read the comment docs.

@owulveryck
Copy link
Owner

Fix with PR #132

@owulveryck owulveryck merged commit f00a765 into master Sep 20, 2019
@owulveryck owulveryck deleted the add-constant branch September 20, 2019 15:50
owulveryck pushed a commit that referenced this pull request Sep 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants