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

fix bug in roi pooling forwarding with cpu #1273

Merged
merged 3 commits into from
Jul 9, 2016

Conversation

apple2373
Copy link
Contributor

I implemented fast r-cnn https://github.com/apple2373/chainer-simple-fast-rnn, and realized roi pooling with cpu does not work. It always outputs only one roi when forwarding. I just copied two lines from GPU implementation. Also I am not sure backward function is also wrong or not because I only use forwarding now.

@okuta okuta added the cat:bug Bug report or fix. label Jun 14, 2016
@okuta
Copy link
Member

okuta commented Jun 14, 2016

Thank you for your PR.
Would you write the test?

@okuta okuta self-assigned this Jun 14, 2016
@apple2373
Copy link
Contributor Author

apple2373 commented Jun 14, 2016

Unfortunately I am not familiar with software testing or software engineering. It seems that roi pooling already has the test: https://github.com/apple2373/chainer/blob/c4b5082e3b0d2973152022aecefb07486b806f5e/tests/chainer_tests/functions_tests/pooling_tests/test_roi_pooling_2d.py

I want to ask inputs from @wkentaro who is the developer of this layer. I guess this is the easy mistake to understand for him.

@wkentaro
Copy link
Contributor

The code looks good to me.
But I'm not sure how to test this.. the test_roi_pooling_2d.py already has test_forward_cpu_gpu_equal and it passed unexpectedly..

@mitmul
Copy link
Member

mitmul commented Jun 23, 2016

I came across the same error.

@apple2373 As for the test, how about modifying this line from N = 4 to N = 3 here?: pooling_tests/test_roi_pooling_2d.py#L17
The number of images in a minibatch and number of rois are incidentally same. That's why the test was passed I think, so please consider to try this modification.

@t-abe
Copy link
Contributor

t-abe commented Jun 30, 2016

mitmul's idea looks good for testing this PR.
@apple2373 Could you modify the test as he mentioned?

@apple2373
Copy link
Contributor Author

I really do not know about software testing. Is it ok just to modify the test_roi_pooling_2d.py as mitmul mentioned, and add it to this PR?

@t-abe
Copy link
Contributor

t-abe commented Jun 30, 2016

Yes. You can add commits into fix_roi_pooling_cpu branch, then this PR is automatically updated.

@apple2373
Copy link
Contributor Author

I did it.

@t-abe
Copy link
Contributor

t-abe commented Jun 30, 2016

@apple2373
Copy link
Contributor Author

I modified cpu backward in the same way.

@t-abe
Copy link
Contributor

t-abe commented Jul 1, 2016

Thank you.
The test still failed. I am going to check the code.
Could you do it too?
You can run a test by $ nosetests -v tests/chainer_tests/function_tests/pooling_tests/....py.

@apple2373
Copy link
Contributor Author

I am not sure what is going on but the test will end up "RuntimeError: CUDA environment is not correctly set up". Maybe test cannot be run only with CPU. I'll try to run on GPU server later.

@t-abe
Copy link
Contributor

t-abe commented Jul 1, 2016

You can run tests only on cpu by giving -a '!gpu' option to nosetests.
And you can see the test results here: https://travis-ci.org/pfnet/chainer/jobs/141322278 .

@apple2373
Copy link
Contributor Author

Thank you. I was able to reproduce test failure on my laptop, but I do not think I can fix it because I do not fully understand the implementation.

@t-abe
Copy link
Contributor

t-abe commented Jul 4, 2016

I fixed the bug and open PR #1352.

@unnonouno unnonouno merged commit 4055bec into chainer:master Jul 9, 2016
unnonouno added a commit that referenced this pull request Jul 9, 2016
fix bug in roi pooling forwarding with cpu (continued from #1273)
@okuta okuta modified the milestone: Closed issues and PRs Jul 26, 2016
hvy pushed a commit that referenced this pull request Nov 29, 2018
Recover StandardUpdater converter backward compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:bug Bug report or fix.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants