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

[Relay] Keras frontend upsample and 1 channel conv2d fixes #3937

Merged
merged 3 commits into from Sep 18, 2019

Conversation

@jwfromm
Copy link
Contributor

commented Sep 11, 2019

This Discussion post found two bugs in Relay. The first was a silly mistake I introduced with the Resize rework (#3788 ) caused by defaulting to the wrong layout for upsample in the Keras frontend. I've resolved this issue and fixed the corresponding test. The second bug occurs when a convolution with 1 filter attempts to determine the shape of its weights. Relay checks for group convolutions by seeing if the number of channels equals the group size, however we obviously shouldn't if the number of channels is 1. I've added a new test to check for this failure in the future.

@jwfromm jwfromm changed the title [Relay] Keras Frontend Upsample and Conv with channels=1 bug fixes [Relay] Keras frontend upsample and 1 channel conv2d fixes Sep 11, 2019

@tqchen

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

@kazum please manage the PR , @jwfromm please request reviews from folks by tagging them next time

@jwfromm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

@kazum, can you take a look? These are pretty minor fixes so it should be quick.

@jwfromm

This comment has been minimized.

Copy link
Contributor Author

commented Sep 17, 2019

@masahi, maybe you can take a look instead?

@jwfromm jwfromm closed this Sep 17, 2019

@jwfromm jwfromm reopened this Sep 17, 2019

@kazum

This comment has been minimized.

Copy link
Member

commented Sep 17, 2019

@jwfromm, sorry for being late. I'll take a look today.

@kazum
kazum approved these changes Sep 18, 2019
Copy link
Member

left a comment

Looks good, thanks @jwfromm !

@kazum kazum merged commit de12376 into dmlc:master Sep 18, 2019

5 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
windows_mac_build Build #20190917.10 succeeded
Details
windows_mac_build (MacOS_XCode9) MacOS_XCode9 succeeded
Details
windows_mac_build (Windows_VS2017_x64) Windows_VS2017_x64 succeeded
Details
windows_mac_build (Windows_VS2017_x86) Windows_VS2017_x86 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.