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
Fix auto_pad shape inference bug #2028
Conversation
Merge master
Merge master
fail_shape_inference("Stride is bigger than Kernel shape"); | ||
} | ||
int64_t total_pad = residual == 0 ? kernel_shape[i] - stride : kernel_shape[i] - residual; | ||
if (total_pad < 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Just 1 naive question - do we really need this mutation ? Even if total pads is negative, in reality, this means just ignoring a few values in the raw data ("unpad" a few values) right ? Also, I think it should not affect the output dim value when we use the formula - floor((in_dim - kernel_shape + total_pad) / s) + 1. Does this seem reasonable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the corner cases and I think probably there is no impact if you leave total_pad as negative value, but for simplicity and clarity, it's better to keep it as non-negative value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@linkerzhang Please review this PR
can you also add a test case to cover this bug case please? Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
* fix auto_pad shape inference bug * add test case for the case stride bigger than kernel shape * Update shape_inference_test.py
* fix auto_pad shape inference bug * add test case for the case stride bigger than kernel shape * Update shape_inference_test.py
No description provided.