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 negative pad values #7375

Merged
merged 10 commits into from
Feb 5, 2021
Merged

Support negative pad values #7375

merged 10 commits into from
Feb 5, 2021

Conversation

codeislife99
Copy link
Contributor

Negative pad values error out currently. This PR removes it and handles the error correctly for negative pad values.

@codeislife99
Copy link
Contributor Author

@anijain2305
Copy link
Contributor

Please add a unit test

Copy link
Contributor

@trevor-m trevor-m left a comment

Choose a reason for hiding this comment

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

Thanks @codeislife99 ! It looks good to me, but could you also add a test case in test_op_level2.py?

@comaniac comaniac added the status: need test case need test cases to cover the change label Jan 29, 2021
@codeislife99
Copy link
Contributor Author

Added relevant test cases.

@codeislife99
Copy link
Contributor Author

codeislife99 commented Jan 30, 2021

@anijain2305 @trevor-m
Please re-review.
One dilemna here is that when shape is dynamic I cannot check if the existing shape + padding would be greater than zero in infertype. This will lead to a memory error during executing when it wont be able to create memory for negative tensor sizes. And the error would be quite bad because the developer/user would not have a clue where it is coming from in a big model.

Does anyone have any idea of how to solve this ? Should we display a warning whenever we encounter negative padding ?

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

When handling the dynamic shape, you need to improve the shape function to check if the shape after padding is still legal on the fly. If it cannot be simply covered in this PR, we can focus on the static shape first and still throw out errors for negative padding values when the shape is dynamic.

tests/python/relay/test_op_level2.py Outdated Show resolved Hide resolved
@comaniac comaniac added status: review in progress and removed status: need test case need test cases to cover the change labels Jan 30, 2021
Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@anijain2305 anijain2305 left a comment

Choose a reason for hiding this comment

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

LGTM

@anijain2305 anijain2305 merged commit c118b08 into apache:main Feb 5, 2021
@anijain2305
Copy link
Contributor

Thanks @codeislife99 @comaniac @trevor-m This is merged

alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 11, 2021
* Support negative pad values

* Update test_op_level2.py

* Update pad.cc

* Update test_op_level2.py

* PR Comments

* Update pad.cc

* Address PR Comments

* CI Error

* CI Error

* CI Error

Co-authored-by: Ubuntu <[email protected]>
Lokiiiiii pushed a commit to Lokiiiiii/tvm that referenced this pull request Mar 2, 2021
* Support negative pad values

* Update test_op_level2.py

* Update pad.cc

* Update test_op_level2.py

* PR Comments

* Update pad.cc

* Address PR Comments

* CI Error

* CI Error

* CI Error

Co-authored-by: Ubuntu <[email protected]>
trevor-m pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2021
* Support negative pad values

* Update test_op_level2.py

* Update pad.cc

* Update test_op_level2.py

* PR Comments

* Update pad.cc

* Address PR Comments

* CI Error

* CI Error

* CI Error

Co-authored-by: Ubuntu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants