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.

Sign up for GitHub

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

Jump to bottom

[Bugfix] Bilinear resize bug fix from PR #2777 #2857

Merged
merged 7 commits into from
Apr 2, 2019
Merged

[Bugfix] Bilinear resize bug fix from PR #2777 #2857

merged 7 commits into from
Apr 2, 2019

Conversation

Laurawly
Copy link
Contributor

@FrozenGene Please review.

@FrozenGene
Copy link
Member

Thanks @Laurawly generally looks good to me and could solve the issues mentioned. However, could we add one unit testing of resize bilinear to compare MXNet forward? I think it will be more nice.

@Laurawly
Copy link
Contributor Author

@FrozenGene Does the test look good to you or more tests are needed?

FrozenGene
FrozenGene reviewed Mar 22, 2019
View reviewed changes
tests/python/frontend/mxnet/test_forward.py
@@ -464,6 +464,12 @@ def verify(data_shape, weight_shape):
verify((2, 2), (4, 5))
verify((2, 3, 4), (4, 5))

def test_forward_bilinear_resize():
Copy link
Member

Choose a reason for hiding this comment

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

I think we can provide one new more unit test with scale_height and scale_width to cover our implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does CI has the latest mxnet installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just try and see if it gives me error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FrozenGene seems https://mxnet.apache.org/api/python/symbol/contrib.html?highlight=bilinearresize#mxnet.symbol.contrib.BilinearResize2D is not giving the correct guidance, it's not related to mxnet version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FrozenGene So I confirmed that only Mxnet 1.5 fixes the bugs. If CI can update mxnet, we can then have this test.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add this in the comment of this issue?

@Laurawly
Copy link
Contributor Author

@FrozenGene Mxnet gives me no "scale_height/width" parameter error locally. I think maybe it depends on the version of Mxnet.

@FrozenGene
Copy link
Member

FrozenGene commented Mar 22, 2019 •
edited
Loading

@Laurawly I use MXNet 1.4 and also get the same error. Seems that it is MXNet's doc issue. I think we test height width is enough currently. However, could we construct another testing? i.e. currently , we are data -> resize bilinear , data is variable. We could add data -> add -> resize bilinear. i.e. resize bilinear accepts one op's output. Then we could cover all conditions of input.

@Laurawly
Copy link
Contributor Author

Laurawly commented Mar 22, 2019 •
edited
Loading

@Laurawly I use MXNet 1.4 and also get the same error. Seems that it is MXNet's doc issue. I think we test height width is enough currently. However, could we construct another testing? i.e. currently , we are data -> resize bilinear , data is variable. We could add data -> add -> resize bilinear. i.e. resize bilinear accepts one op's output. Then we could cover all conditions of input.

I confirmed with mxnet developers and confirmed that it's a bug. Mxnet 1.5 solves this issue. If we could upgrade ci to use mxnet 1.5 (pre-release) problem will be solved @tqchen .

@Laurawly Laurawly closed this Mar 22, 2019
@Laurawly Laurawly deleted the bilinear_resize branch March 22, 2019 20:28
@Laurawly Laurawly restored the bilinear_resize branch March 22, 2019 20:41
@Laurawly Laurawly reopened this Mar 22, 2019
@Laurawly
Copy link
Contributor Author

@FrozenGene @tqchen Is it possible to update the CI to use mxnet 1.5? Or I remove the test for now?

@FrozenGene
Copy link
Member

I personally suggest remove the test of scale_height / scale_width. Because current stable version of MXNet doesn’t have it. Also want to hear @tqchen’s comment.

@tqchen
Copy link
Member

tqchen commented Mar 25, 2019

Let us remove the test

@Laurawly
Copy link
Contributor Author

@FrozenGene Test removed.

@yzhliu
Copy link
Member

yzhliu commented Mar 27, 2019

Please kick the CI again.

@Laurawly Laurawly force-pushed the bilinear_resize branch 2 times, most recently from ffc2e91 to 88afbdc Compare March 29, 2019 20:20
icemelon
icemelon reviewed Mar 29, 2019
View reviewed changes
tests/python/frontend/mxnet/test_forward.py Outdated
@@ -506,4 +513,8 @@ def test_forward_smooth_l1():
test_forward_broadcast_axis()
test_forward_full()
test_forward_embedding()
<<<<<<< a07df95a837dceb70d8f36c33fe680a913faf2b9
Copy link
Member

@icemelon icemelon Mar 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Need to remove these lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reminding!

Laurawly
Laurawly commented Apr 2, 2019
View reviewed changes
tests/python/frontend/mxnet/test_forward.py
@@ -505,6 +505,12 @@ def verify(xshape, yshape, y_data):
verify((2, 2), (2, 3), [[1, 1, 0], [0, 1, 0]])
verify((2, 2, 2), (2, 2), [[0, 1], [1, 0]])

def test_forward_bilinear_resize():
# add tests including scale_height and scale_width when mxnet is updated to version 1.5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@icemelon9 Please check if this looks ok.

Copy link
Member

Choose a reason for hiding this comment

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

Already approved. Let's wait for CI to complete.

@icemelon
Copy link
Member

icemelon commented Apr 2, 2019

Thanks everyone. this is now merged

@icemelon icemelon merged commit 1dab4dc into apache:master Apr 2, 2019
ajtulloch pushed a commit to ajtulloch/tvm that referenced this pull request Apr 5, 2019
@Laurawly @ajtulloch
[Bugfix] Bilinear resize bug fix from PR apache#2777 (apache#2857)
9550f62 
* error fixed

* rename

* solve conlicts with master

* more test added

* fix error

* remove test

* comment addressed
wweic pushed a commit to wweic/tvm that referenced this pull request Apr 7, 2019
@Laurawly @wweic
[Bugfix] Bilinear resize bug fix from PR apache#2777 (apache#2857)
21443b2 
* error fixed

* rename

* solve conlicts with master

* more test added

* fix error

* remove test

* comment addressed
wweic pushed a commit to wweic/tvm that referenced this pull request Apr 7, 2019
@Laurawly @wweic
[Bugfix] Bilinear resize bug fix from PR apache#2777 (apache#2857)
ce8f310 
* error fixed

* rename

* solve conlicts with master

* more test added

* fix error

* remove test

* comment addressed
wweic pushed a commit to wweic/tvm that referenced this pull request Apr 8, 2019
@Laurawly @wweic
[Bugfix] Bilinear resize bug fix from PR apache#2777 (apache#2857)
c377338 
* error fixed

* rename

* solve conlicts with master

* more test added

* fix error

* remove test

* comment addressed
wweic pushed a commit to wweic/tvm that referenced this pull request Apr 10, 2019
@Laurawly @wweic
[Bugfix] Bilinear resize bug fix from PR apache#2777 (apache#2857)
1ea177f 
* error fixed

* rename

* solve conlicts with master

* more test added

* fix error

* remove test

* comment addressed
wweic pushed a commit to neo-ai/tvm that referenced this pull request Apr 11, 2019
@Laurawly @wweic
[Bugfix] Bilinear resize bug fix from PR apache#2777 (apache#2857)
2e5a5c6 
* error fixed

* rename

* solve conlicts with master

* more test added

* fix error

* remove test

* comment addressed
@yzhliu yzhliu mentioned this pull request Nov 11, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@icemelon icemelon icemelon approved these changes

@FrozenGene FrozenGene FrozenGene approved these changes

Assignees

@icemelon icemelon

Labels
status: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Laurawly @FrozenGene @tqchen @yzhliu @icemelon