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.

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

Support cover_all=True on Unpooling2D in exporting to ONNX #8391

Merged
merged 4 commits into from
Nov 12, 2019

Conversation

disktnk
Copy link
Member

@disktnk disktnk commented Nov 8, 2019

Support cover_all=True on Chainer's Unpool2d. Simply recalculate output size. Interpolation logic is differ between Chainer and ONNXRuntime, and output values checking is skipped.

@disktnk disktnk added the ONNX Related to ONNX label Nov 8, 2019
@take-cheeze take-cheeze self-assigned this Nov 8, 2019
@take-cheeze
Copy link
Member

Jenkins. test this please!

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 69d3cc2, target branch master) succeeded!

Copy link
Member

@shinh shinh left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -24,6 +24,11 @@
'in_shape': (1, 3, 6, 5, 4), 'args': [3, 2, 1], 'cover_all': True},
{'op_name': 'unpooling_2d',
'in_shape': (1, 3, 6, 6), 'args': [3, None, 0], 'cover_all': False},
# when cover_all=True, interpolation between Chainer and ONNXRuntime
Copy link
Member

Choose a reason for hiding this comment

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

I feel we need a TODO for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

add TODO, thx

Copy link
Member

@take-cheeze take-cheeze left a comment

Choose a reason for hiding this comment

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

CI is OK so LGTM 👍

@take-cheeze
Copy link
Member

Jenkins, test this please!

@take-cheeze take-cheeze added the st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. label Nov 12, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 72a2edc, target branch master) succeeded!

@mergify mergify bot merged commit dc68465 into chainer:master Nov 12, 2019
@disktnk disktnk deleted the fix/unpooling-coverall branch November 12, 2019 04:46
@kmaehashi kmaehashi added this to the v7.0.0 milestone Dec 5, 2019
@kmaehashi kmaehashi added the cat:enhancement Implementation that does not break interfaces. label Dec 5, 2019
@emcastillo emcastillo changed the title Support cover_all=True on Unpooling2D in exporting to ONNX Support cover_all=True on Unpooling2D in exporting to ONNX Dec 5, 2019
@kmaehashi kmaehashi mentioned this pull request Dec 5, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 72a2edc, target branch master) succeeded!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:enhancement Implementation that does not break interfaces. ONNX Related to ONNX st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants