Codecov Report

Merging #52 into features/region-proposal-network-losses will increase coverage by 0.2%. The diff coverage is 80.39%.

@@                            Coverage Diff                             @@
##           features/region-proposal-network-losses      #52     +/-   ##
==========================================================================
+ Coverage                                    65.99%   66.19%   +0.2%     
==========================================================================
  Files                                           17       17             
  Lines                                          644      642      -2     
==========================================================================
  Hits                                           425      425             
+ Misses                                         219      217      -2

Impacted Files	Coverage Δ
keras_rcnn/preprocessing/_object_detection.py	`0% <0%> (ø)`	:arrow_up:
keras_rcnn/losses/rpn.py	`100% <100%> (ø)`	:arrow_up:
keras_rcnn/models.py	`100% <100%> (ø)`	:arrow_up:
keras_rcnn/classifiers/resnet.py	`93.75% <100%> (-0.7%)`	:arrow_down:
keras_rcnn/backend/common.py	`100% <100%> (ø)`	:arrow_up:
...s_rcnn/layers/object_detection/_object_proposal.py	`100% <100%> (ø)`	:arrow_up:
keras_rcnn/backend/tensorflow_backend.py	`100% <100%> (ø)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9a7a4c1...b44d23d. Read the comment docs.

hgaiser commented 7 years ago

With the exception of the TODO's mentioned in the code, I think the ObjectProposal layer should work (in theory) with this PR.

Something I noticed, for which I didn't have a fix yet, is that the number of maximum NMS proposals differs depending on whether we are training or testing. Currently in keras-rcnn the value is set to 300, always.

A bigger question is hiding in there, which is what do we do about configuration such as this?

jhung0 commented 7 years ago

Ready to merge? Yes, currently we have much fewer config options than the original implementation. We could create a class (like in https://github.com/yhenon/keras-frcnn/blob/master/keras_frcnn/config.py), but I guess that's not ideal @0x00b1 ?

hgaiser commented 7 years ago

@jhung0 yeah I'm done :) if you agree with the changes I've made then it is good to merge.

I agree, a decision should be made on how to handle configuration.

hgaiser commented 7 years ago

What I think is still missing in this PR:

I stopped porting anchor_target_layer until here. The rest of the forward method also needs to be ported.
The losses need to be implemented as Keras layers (a la https://github.com/fchollet/keras/blob/master/examples/variational_autoencoder.py#L46-L62)

I believe these two things should be enough to attempt to train the RPN part of RCNN. For the head of RCNN, proposal_target_layer is also required (preferably as one layer?) and additional loss functions need to be made.

In addition it wouldn't hurt to have unit tests and documentation for all this :)

broadinstitute / keras-rcnn

Region Proposal Network (RPN) corrections #52

Codecov Report