[ONNX][TOPI][Relay]Support dilations in pooling operators #7928
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndrewZhaoLuo
force-pushed
the
andrewluo-add-dilations-to-pooling
branch
from
0298d39
to
c655421
Compare
AndrewZhaoLuo
changed the title
AndrewZhaoLuo
changed the title
AndrewZhaoLuo
force-pushed
the
andrewluo-add-dilations-to-pooling
branch
from
800449e
to
62c832d
Compare
|
Moving this back to [WIP], by request I'm adding onnx integration too. |
AndrewZhaoLuo
changed the title
AndrewZhaoLuo
changed the title
AndrewZhaoLuo
force-pushed
the
andrewluo-add-dilations-to-pooling
branch
2 times, most recently
from
8b725eb
to
634253a
Compare
|
Ok ONNX integration is done now, as should be most frontend tests. (We'll see if CI agrees!) |
mbrookhart
approved these changes
Apr 30, 2021
There was a problem hiding this comment.
Overall looks good to me, a couple of minor nitpicks.
There are a lot of changes to alphabetize import order which make the core of the PR kind of hard to read. Did black do that, or did you do it manually? If black did it, what version of black are you running?
Comment on lines
35
to
40
| output_height = ( | ||
| (shape[1] - (sizes[0] - 1) * dilation[0] - 1 + padding[0] + padding[2]) / strides[0] | ||
| ) + 1 | ||
| output_width = ( | ||
| (shape[2] - (sizes[1] - 1) * dilation[1] - 1 + padding[1] + padding[3]) / strides[1] | ||
| ) + 1 |
There was a problem hiding this comment.
nitpick: the way this got formatted is hard to read. Perhaps an extra set of parentheses outside would help?
There was a problem hiding this comment.
I've factored out some terms into a "receptive_field" variable. It was the prettiest way I could figure out.
tests/python/relay/test_op_level5.py
Outdated
Comment on lines
674
to
677
| tvm.testing.assert_allclose(op_res1.asnumpy(), ref_res, rtol=1e-4) | ||
| tvm.testing.assert_allclose(op_res1.asnumpy(), ref_res, rtol=2e-4) | ||
| intrp2 = relay.create_executor("debug", device=dev, target=target) | ||
| op_res2 = intrp2.evaluate(func)(np_data, np_rois) | ||
| tvm.testing.assert_allclose(op_res2.asnumpy(), ref_res, rtol=1e-4) | ||
| tvm.testing.assert_allclose(op_res2.asnumpy(), ref_res, rtol=2e-4) |
There was a problem hiding this comment.
Why did your changes to pooling change the precision of roi_align?
There was a problem hiding this comment.
I'm not sure actually. Taking a quick look at roi_align, I don't think it should be affected at all. I'm going to try to up the strictness of the test back to before and see if we have the same issues.
There was a problem hiding this comment.
Ok it passes still with the original tolerances, it was probably due to another bug that was fixed a long time ago
dilations -> dilation to match old field names in conv fix python interface into new relay nodes fix order of arguments update type relation for dilations change topi interface to use dilations
use generic poolnd instead of 2d implementation for topi remove old pooling topi
change pool -> pool2d, make topi tests work now make op level 2 pass with interface changes fix dilation being hardcoded to 1 proper calculation for avgs among dilations proper avg pool padding behavior change name of pool test to pool2d test
more fixes to edge cases for poolnd, delete old versions replace topi tests with new baseline python version clean up tests make tests more readable kind of add dilation topi tests FINALLY remove see_pool.py remove dilation from grad
add relay dilation tests, FINALLY add some comments to testing code linting and formatting add ASF header make 10/10 for black formatting lol more appeasing the formatting gods wow add parameters to documentation fix test import Jostle CI fix more broken unit tests using old version of pool fix wrong var used for bound calc add dilation to arm tests add docstring to python make funcs
formatting more formatting relax constraints on test to make it pass relax more constraints fix some pytorch frontend errors fix error better test conditions jostle build
jostle build cleaner pool condition remove see_pool.py again
blacking files black file
AndrewZhaoLuo
force-pushed
the
andrewluo-add-dilations-to-pooling
branch
from
6e470d2
to
3d87f4f
Compare
|
Thanks @AndrewZhaoLuo |
umangyadav
pushed a commit
to umangyadav/tvm
that referenced
this pull request
May 5, 2021
* change more pooling operators dilations -> dilation to match old field names in conv fix python interface into new relay nodes fix order of arguments update type relation for dilations change topi interface to use dilations * spooky, there are two implementations! Change to 1 topi use generic poolnd instead of 2d implementation for topi remove old pooling topi * rename pool --> pool2d in topi change pool -> pool2d, make topi tests work now make op level 2 pass with interface changes fix dilation being hardcoded to 1 proper calculation for avgs among dilations proper avg pool padding behavior change name of pool test to pool2d test * add poolnd baseline implementation more fixes to edge cases for poolnd, delete old versions replace topi tests with new baseline python version clean up tests make tests more readable kind of add dilation topi tests FINALLY remove see_pool.py remove dilation from grad * fix subtle implementation detail between topi and baseline python pool op * rewrite tests to be more generic for relay pooling ops add relay dilation tests, FINALLY add some comments to testing code linting and formatting add ASF header make 10/10 for black formatting lol more appeasing the formatting gods wow add parameters to documentation fix test import Jostle CI fix more broken unit tests using old version of pool fix wrong var used for bound calc add dilation to arm tests add docstring to python make funcs * fix pattern utils out of place args * properly forward more tests to use dilations in pooling formatting more formatting relax constraints on test to make it pass relax more constraints fix some pytorch frontend errors fix error better test conditions jostle build * fix padding bug with ceil mode jostle build cleaner pool condition remove see_pool.py again * add dilations field to onnx importer blacking files black file * address matthew's comments
trevor-m
pushed a commit
to trevor-m/tvm
that referenced
this pull request
May 6, 2021
* change more pooling operators dilations -> dilation to match old field names in conv fix python interface into new relay nodes fix order of arguments update type relation for dilations change topi interface to use dilations * spooky, there are two implementations! Change to 1 topi use generic poolnd instead of 2d implementation for topi remove old pooling topi * rename pool --> pool2d in topi change pool -> pool2d, make topi tests work now make op level 2 pass with interface changes fix dilation being hardcoded to 1 proper calculation for avgs among dilations proper avg pool padding behavior change name of pool test to pool2d test * add poolnd baseline implementation more fixes to edge cases for poolnd, delete old versions replace topi tests with new baseline python version clean up tests make tests more readable kind of add dilation topi tests FINALLY remove see_pool.py remove dilation from grad * fix subtle implementation detail between topi and baseline python pool op * rewrite tests to be more generic for relay pooling ops add relay dilation tests, FINALLY add some comments to testing code linting and formatting add ASF header make 10/10 for black formatting lol more appeasing the formatting gods wow add parameters to documentation fix test import Jostle CI fix more broken unit tests using old version of pool fix wrong var used for bound calc add dilation to arm tests add docstring to python make funcs * fix pattern utils out of place args * properly forward more tests to use dilations in pooling formatting more formatting relax constraints on test to make it pass relax more constraints fix some pytorch frontend errors fix error better test conditions jostle build * fix padding bug with ceil mode jostle build cleaner pool condition remove see_pool.py again * add dilations field to onnx importer blacking files black file * address matthew's comments
trevor-m
pushed a commit
to trevor-m/tvm
that referenced
this pull request
May 6, 2021
* change more pooling operators dilations -> dilation to match old field names in conv fix python interface into new relay nodes fix order of arguments update type relation for dilations change topi interface to use dilations * spooky, there are two implementations! Change to 1 topi use generic poolnd instead of 2d implementation for topi remove old pooling topi * rename pool --> pool2d in topi change pool -> pool2d, make topi tests work now make op level 2 pass with interface changes fix dilation being hardcoded to 1 proper calculation for avgs among dilations proper avg pool padding behavior change name of pool test to pool2d test * add poolnd baseline implementation more fixes to edge cases for poolnd, delete old versions replace topi tests with new baseline python version clean up tests make tests more readable kind of add dilation topi tests FINALLY remove see_pool.py remove dilation from grad * fix subtle implementation detail between topi and baseline python pool op * rewrite tests to be more generic for relay pooling ops add relay dilation tests, FINALLY add some comments to testing code linting and formatting add ASF header make 10/10 for black formatting lol more appeasing the formatting gods wow add parameters to documentation fix test import Jostle CI fix more broken unit tests using old version of pool fix wrong var used for bound calc add dilation to arm tests add docstring to python make funcs * fix pattern utils out of place args * properly forward more tests to use dilations in pooling formatting more formatting relax constraints on test to make it pass relax more constraints fix some pytorch frontend errors fix error better test conditions jostle build * fix padding bug with ceil mode jostle build cleaner pool condition remove see_pool.py again * add dilations field to onnx importer blacking files black file * address matthew's comments
trevor-m
pushed a commit
to trevor-m/tvm
that referenced
this pull request
May 6, 2021
* change more pooling operators dilations -> dilation to match old field names in conv fix python interface into new relay nodes fix order of arguments update type relation for dilations change topi interface to use dilations * spooky, there are two implementations! Change to 1 topi use generic poolnd instead of 2d implementation for topi remove old pooling topi * rename pool --> pool2d in topi change pool -> pool2d, make topi tests work now make op level 2 pass with interface changes fix dilation being hardcoded to 1 proper calculation for avgs among dilations proper avg pool padding behavior change name of pool test to pool2d test * add poolnd baseline implementation more fixes to edge cases for poolnd, delete old versions replace topi tests with new baseline python version clean up tests make tests more readable kind of add dilation topi tests FINALLY remove see_pool.py remove dilation from grad * fix subtle implementation detail between topi and baseline python pool op * rewrite tests to be more generic for relay pooling ops add relay dilation tests, FINALLY add some comments to testing code linting and formatting add ASF header make 10/10 for black formatting lol more appeasing the formatting gods wow add parameters to documentation fix test import Jostle CI fix more broken unit tests using old version of pool fix wrong var used for bound calc add dilation to arm tests add docstring to python make funcs * fix pattern utils out of place args * properly forward more tests to use dilations in pooling formatting more formatting relax constraints on test to make it pass relax more constraints fix some pytorch frontend errors fix error better test conditions jostle build * fix padding bug with ceil mode jostle build cleaner pool condition remove see_pool.py again * add dilations field to onnx importer blacking files black file * address matthew's comments
trevor-m
pushed a commit
to neo-ai/tvm
that referenced
this pull request
May 11, 2021
* change more pooling operators dilations -> dilation to match old field names in conv fix python interface into new relay nodes fix order of arguments update type relation for dilations change topi interface to use dilations * spooky, there are two implementations! Change to 1 topi use generic poolnd instead of 2d implementation for topi remove old pooling topi * rename pool --> pool2d in topi change pool -> pool2d, make topi tests work now make op level 2 pass with interface changes fix dilation being hardcoded to 1 proper calculation for avgs among dilations proper avg pool padding behavior change name of pool test to pool2d test * add poolnd baseline implementation more fixes to edge cases for poolnd, delete old versions replace topi tests with new baseline python version clean up tests make tests more readable kind of add dilation topi tests FINALLY remove see_pool.py remove dilation from grad * fix subtle implementation detail between topi and baseline python pool op * rewrite tests to be more generic for relay pooling ops add relay dilation tests, FINALLY add some comments to testing code linting and formatting add ASF header make 10/10 for black formatting lol more appeasing the formatting gods wow add parameters to documentation fix test import Jostle CI fix more broken unit tests using old version of pool fix wrong var used for bound calc add dilation to arm tests add docstring to python make funcs * fix pattern utils out of place args * properly forward more tests to use dilations in pooling formatting more formatting relax constraints on test to make it pass relax more constraints fix some pytorch frontend errors fix error better test conditions jostle build * fix padding bug with ceil mode jostle build cleaner pool condition remove see_pool.py again * add dilations field to onnx importer blacking files black file * address matthew's comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds dilations to pooling operations:
As an example of what this means. Assume we have a tensor with layout N, C, H, W.
We have a pooling kernel of k_h x k_w and corresponding strides s_y and s_x.
Let d_y and d_x be the associated dilations for the operation. Previously we had d_y and d_x implicitly be 1. An average pool is then:
MaxPool in ONNX supports dilation so this is important. It's also potentially useful for writing quantized conv operators.
General:
Relay:
TopI:
Onnx:
cc @electriclilies @mbrookhart