-
Notifications
You must be signed in to change notification settings - Fork 222
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
[WIP] Region Proposal Network (RPN) classification and regression losses #51
Conversation
Awesome. Thanks, @hgaiser! Two immediate questions after a quick review:
What’s your time zone? Let’s find a time to talk tomorrow (or Friday). |
@hgaiser (et al.) I created a |
It doesn't show in this PR, but since the base anchors have nothing to do with the input, I think it'd be better to generate them while creating the model, ie., offline.
Sure, I'll do it in this comment:
Basically there are three options to do loss for RPN, none of which are really nice. Keras just doesn't have the tools to do loss computations like we need (see keras-team/keras#7395). The options are :
Preferably there will be a change in how Keras forces loss to be computed in such a strict way. I wrote an issue (keras-team/keras#7395) about it but so far no responses.. Until then, option 3 seems like our best bet. As for the training process itself, I had written a data generator that retrieves images and annotations from our server. It then serves the image batch and ground truth boxes. The training command would look something like this:
No I think we should stick with
My timezone is UTC/GMT +2. You're in the states I guess ? :) |
6284600
to
d738b0e
Compare
d738b0e
to
5b764be
Compare
I pushed an updated version, I mistakenly thought |
@hgaiser We used to have an anchor_target_layer alternative. But we decided to move generating anchor target to the RPN loss #7 . However, this direction didn't go well as expected because of the check for |
Hey, @hgaiser: I really appreciate your feedback! I think we’re all circling around the same issues.
It’s certainly easier! Unfortunately, the downside is that you’re removing needed functionality from the compute graph produced by your Keras backend. Initially, we planned (and added) anchor generation inside the
I’m probably misunderstanding your meaning, but if you’re talking about the shapes produced by the reference bounding box convolutional layer and the objectness convolutional layer, I believe they can be dynamically inferred:
and:
Is that what you mean?
I’m curious, what do you think about this? I really like the idea of treating the RPN losses as regularization terms. Moreover, it’d simplify the training and testing differences.
Yep! I'm in Boston. |
Yeah, I agree. I think it was worth a shot, but it feels like we need to reintroduce a layer. What do you think @hgaiser? Is anybody excited to work on this? |
Ah so with an eye on deployment on phones or the cloud or anything basically that doesn't have
I'm not sure about those shapes, but even if the shapes can be inferred, its content cannot (ie. which of the anchors are labelled as
How would this work? Don't regularization functions receive weights and biases from a specific layer? Would you make a custom layer where the prediction and target values are 'weights' so that you can perform regularization on those 'weights' ? This seems like a big workaround. |
Exactly. Unfortunately, this may prove (or has proved) too ambitious for the moment. Especially if we’re interested in a functional release in the next few weeks.
What do you mean by “the position it has in master?”
Good point.
Yeah, it’s far from ideal. And the implementation would be clumsy. @hgaiser What strategy would you choose? Generate anchors from the ObjectDetectionGenerator (similar to the existing implementation inspired by @yhenon)? Fix Keras’ inflexibility around losses? Nevertheless, I think we should probably choose something sooner than later so we can start testing other parts (e.g. augmentation, RCNN, Mask RCNN, etc.). |
Ah I see that part isn't in
Preferably we can define the anchors as constants somehow, which are only computed once. It's not a big deal so if this proves to be difficult, generating them every inference is also fine. Fixing Keras' inflexibility around losses is probably going to be difficult, since it's a major overhaul of their API. Making an addition to the Keras API, keeping the original intact, is an option, but will probably also take some time before that is approved and merged. I think the best option is to implement it using the " |
I will close this PR as it has served its purpose. |
Not sure if this (PR) is the best way to show what I had in mind, but lets give it a shot :)
This PR is mainly intended to show how I more or less had implemented RPN losses in my own port. I'm guessing the losses don't make sense at the moment, they are only there to convey the idea. Mathematically they probably make no sense at the moment because I believe it receives different inputs.
Also I changed the ResNet classifier according to the changes in broadinstitute/keras-resnet#24, I'm not sure if the existing ResNet classifier in
keras-rcnn
was correct.Also, the RPN layers appeared to miss an initial convolutional layer, called
rpn_conv_3x3
in the Caffe prototxts. In addition, the convolutional layer for rpn class scores gave as outputnum_anchors
, but it should givenum_anchors * 2
, since it classifies two classes (fg/bg).These are the main contributions in this PR, aside from some minor corrections here and there.
I would gladly hear what you think @0x00b1 . Maybe we can discuss this further tomorrow, on Slack?