Skip to content

Generate node backend tests for celu operator #2607

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
Merged
merged 13 commits into from
Feb 25, 2020
Merged

Generate node backend tests for celu operator #2607

merged 13 commits into from
Feb 25, 2020

Conversation

TMVector
Copy link
Contributor

Running tools/update_doc.sh from master creates these previously untracked files

@TMVector TMVector requested a review from a team as a code owner February 17, 2020 19:52
@jeremycochoy
Copy link
Contributor

😱 Something is wrong with the Celu backend tests. I will see if I can fix it. Sorry. 😞

@TMVector
Copy link
Contributor Author

No worries @jeremycochoy, happens to everyone :)

I think the problem might actually be with the code which creates the expanded-functions test not handling ref attrs correctly[0]. I'll give fixing it a quick try now in this PR.

[0] https://github.com/onnx/onnx/blob/master/onnx/backend/test/case/node/__init__.py#L59-L63

@TMVector TMVector force-pushed the generate-celu-tests branch from 6150f5b to 462c0aa Compare February 18, 2020 11:02
@jeremycochoy
Copy link
Contributor

Thanks. :)

I am also wondering if the lines at

onnx/onnx/defs/function.cc

Lines 75 to 77 in 807c62c

if (attr_map.count(attr.ref_attr_name())) {
AttributeProto* new_attr = new_node->add_attribute();
new_attr->CopyFrom(attr_map[attr.ref_attr_name()]);
may require some modification too ?

@jeremycochoy
Copy link
Contributor

One last mistake I found that should result in test failing:

onnx/onnx/backend/test/case/node/celu.py

Lines 17 to 21 in c978d10

node = onnx.helper.make_node(
'Celu',
inputs=['X'],
outputs=['Y']
)

The attribute alpha=2 (should be alpha=2.) is not forwarded to the node, so there should be a mismatch between the backend computing with alpha=1. and numpy computing with alpha=2..

TMVector and others added 3 commits February 18, 2020 13:50
@TMVector @jeremycochoy
Fix ref attrs in FunctionExpandHelper
2dd5869 
Co-authored-by: Jeremy Cochoy <jeremy.cochoy@gmail.com>
@TMVector @jeremycochoy
Use correct alpha in celu backend test
8f0cdc6 
Co-authored-by: Jeremy Cochoy <jeremy.cochoy@gmail.com>
@TMVector
Generate node backend tests for celu operator
4f27d0b
@TMVector TMVector force-pushed the generate-celu-tests branch from 316fcec to 4f27d0b Compare February 18, 2020 13:53
@linkerzhang
Copy link
Member

please fix Linux ci failure.

+flake8
2388
./onnx/backend/test/case/node/celu.py:22:18: E251 unexpected spaces around keyword / parameter equals
2389
./onnx/backend/test/case/node/celu.py:22:20: E251 unexpected spaces around keyword / parameter equals

jeremycochoy
jeremycochoy reviewed Feb 20, 2020
View reviewed changes
@prasanthpul prasanthpul added this to the 1.7 milestone Feb 24, 2020
@linkerzhang linkerzhang merged commit 8c5bf9d into onnx:master Feb 25, 2020
@TMVector TMVector deleted the generate-celu-tests branch February 25, 2020 13:40
@TMVector TMVector mentioned this pull request Feb 27, 2020
Closed
jcwchen pushed a commit to jcwchen/onnx that referenced this pull request Sep 23, 2020
@TMVector @jeremycochoy @linkerzhang
Generate node backend tests for celu operator (onnx#2607)
b7e93f6 
* Fix ref attrs in expanded node tests

* Fix ref attrs in FunctionExpandHelper

Co-authored-by: Jeremy Cochoy <jeremy.cochoy@gmail.com>

* Use correct alpha in celu backend test

Co-authored-by: Jeremy Cochoy <jeremy.cochoy@gmail.com>

* Generate node backend tests for celu operator

* Fix type of alpha attribute in celu node backend test

* Fix over-opinionated flake8

* Fixup celu attr type

* Regenerate docs

Co-authored-by: Jeremy Cochoy <jeremy.cochoy@gmail.com>
Co-authored-by: Ke Zhang <kezhan@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@jeremycochoy jeremycochoy jeremycochoy left review comments

@linkerzhang linkerzhang linkerzhang approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.7
Development

Successfully merging this pull request may close these issues.
4 participants
@TMVector @jeremycochoy @linkerzhang @prasanthpul