proposals from RPN per image during training

[chenjoya]

Hi @fmassa , the function select_over_all_levels is modified for single image during training.
The original issue: #672
Thank you ^ ^

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[fmassa]

Hi @chenjoya

This is a breaking change, and I'd rather not break other people's checkpoint as is.

Could you instead add a note to the documentation, mentioning this buggy behavior?

Thanks!

[chenjoya]

Hi @fmassa , you are right, and the change of code needs to be check more.
Therefore I only rectify the README.md, and show how to set FPN_POST_NMS_TOP_N_TRAIN.
Thanks for your advise!

[rodrigoberriel]

@fmassa Would it be okay if we add a flag (e.g., _C.MODEL.RPN.AS_IN_DETECTRON -- hopefully a better name) with default True and add a section (e.g., "Known Issues" or "Known Bugs") to the README describing the default behavior, known side-effects (the one described in this PR) and the workarounds (1. do as recommended in this PR if the user cannot afford to re-train or 2. train in the correct way changing the flag)? Or maybe just a section to the TROUBLESHOOTING.md? I feel like we should provide a "switch" to avoid training like this, since training in the correct way apparently leads to the "same" results; then, it would be up to the user.

What do you think, @chenjoya ?

[fmassa]

just removing this extra space change and this should be good to merge!

[fmassa]

@rodrigoberriel the documentation, after @chenjoya improvements, already makes it fairly clear about this behavior.

Adding one more config argument would be an option to accommodate both behaviors, but it will just make the code more complicated.

[rodrigoberriel]

@fmassa fair enough about the documentation

Would you be willing to review a PR trying to accommodate both behaviors? I think it would be a 5~10 lines change and we could convert this TODO into a NOTE :)

maskrcnn-benchmark/maskrcnn_benchmark/modeling/rpn/inference.py

Lines 154 to 159 in 1714b7c

	# different behavior during training and during testing:
	# during training, post_nms_top_n is over *all* the proposals combined, while
	# during testing, it is over the proposals for each image
	# TODO resolve this difference and make it consistent. It should be per image,
	# and not per batch
	if self.training:

[chenjoya]

@fmassa Thanks for your review. The extra space change has been removed :)

[fmassa]

Thanks!

[fmassa]

@rodrigoberriel sure, why not adding an option (defaulting to the current behavior) that switches between behaviors. PRs welcome! :-)