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

WIP: Correct computing offset of anchors. #1075

Closed
wants to merge 1 commit into from
Closed

Conversation

hgaiser
Copy link
Contributor

@hgaiser hgaiser commented Jul 17, 2019

This PR fixes the computation of the offset of the anchors, thanks to @jonilaserson . This PR fixes #1073 .

This affects all networks trained before this change. They should be retrained after this is merged for it to work with the latest version.

ps. I think we should hold off merging this, until we have a retrained version of the COCO weights.

@hgaiser hgaiser requested a review from vcarpani July 17, 2019 10:23
# compute the offset of the anchors based on the image shape and the feature map shape
# see https://github.com/fizyr/keras-retinanet/issues/1073 for more information
offset_x = (image_shape[1] - (features_shape[1] - 1) * stride) / 2
offset_y = (image_shape[0] - (features_shape[0] - 1) * stride) / 2

Choose a reason for hiding this comment

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

If you want to support python 2.X, better divide by 2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I wish we weren't supporting 2.x, but unfortunately we are :p

Copy link
Contributor

@vcarpani vcarpani left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @jonilaserson for catching this 🥇

@hgaiser before merging let's wait for COCO to be trained both for keras-retinananet and keras-maskrcnn. We should also properly announce this change on the slack channel and bump the release to 0.6.0.

@vcarpani vcarpani changed the title Correct computing offset of anchors. WIP: Correct computing offset of anchors. Jul 17, 2019
@hgaiser
Copy link
Contributor Author

hgaiser commented Jul 18, 2019

I've also modified the tensorflow shift function. Unfortunately we need this function twice, once in numpy and once in tensorflow ops. The evaluation on COCO, using the same weights as before, give the following results:

 Average Precision  (AP) @[ IoU=0.50:0.95 | area=   all | maxDets=100 ] = 0.286
 Average Precision  (AP) @[ IoU=0.50      | area=   all | maxDets=100 ] = 0.530
 Average Precision  (AP) @[ IoU=0.75      | area=   all | maxDets=100 ] = 0.280
 Average Precision  (AP) @[ IoU=0.50:0.95 | area= small | maxDets=100 ] = 0.170
 Average Precision  (AP) @[ IoU=0.50:0.95 | area=medium | maxDets=100 ] = 0.336
 Average Precision  (AP) @[ IoU=0.50:0.95 | area= large | maxDets=100 ] = 0.368
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets=  1 ] = 0.257
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets= 10 ] = 0.424
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets=100 ] = 0.464
 Average Recall     (AR) @[ IoU=0.50:0.95 | area= small | maxDets=100 ] = 0.315
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=medium | maxDets=100 ] = 0.523
 Average Recall     (AR) @[ IoU=0.50:0.95 | area= large | maxDets=100 ] = 0.567

The original results:

 Average Precision  (AP) @[ IoU=0.50:0.95 | area=   all | maxDets=100 ] = 0.350
 Average Precision  (AP) @[ IoU=0.50      | area=   all | maxDets=100 ] = 0.537
 Average Precision  (AP) @[ IoU=0.75      | area=   all | maxDets=100 ] = 0.374
 Average Precision  (AP) @[ IoU=0.50:0.95 | area= small | maxDets=100 ] = 0.191
 Average Precision  (AP) @[ IoU=0.50:0.95 | area=medium | maxDets=100 ] = 0.383
 Average Precision  (AP) @[ IoU=0.50:0.95 | area= large | maxDets=100 ] = 0.472
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets=  1 ] = 0.306
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets= 10 ] = 0.491
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=   all | maxDets=100 ] = 0.533
 Average Recall     (AR) @[ IoU=0.50:0.95 | area= small | maxDets=100 ] = 0.345
 Average Recall     (AR) @[ IoU=0.50:0.95 | area=medium | maxDets=100 ] = 0.577
 Average Recall     (AR) @[ IoU=0.50:0.95 | area= large | maxDets=100 ] = 0.681

There's a difference there, which makes sense. The difference for IoU=0.5 is rather small, but the difference for IoU=0.75 is large (causing the difference in mAP to be relatively large as well). Basically all detections have been shifted a few pixels to the top left corner, which isn't that critical with a lenient overlap threshold of 0.5, but for 0.75 it is more severe.

Next I'll start a training process to finetune the network for this new setting, and add unit tests for this function.

@hgaiser
Copy link
Contributor Author

hgaiser commented Jul 18, 2019

Added unit tests and started training starting from the previous best weights. The first epoch moved the mAP from 0.286 to 0.333, so that's a nice first increase. I'll see how far it gets, hopefully it becomes at least as accurate as the original model :)

I'm awaiting the results of training (and that of keras-maskrcnn) before merging this. We'll have to tag it as a new version as well.

Check #1073 for more
information.
@hgaiser
Copy link
Contributor Author

hgaiser commented Jul 24, 2019

This is still on my radar btw, but it takes a while to retrain. So far the best mAP I got was 0.343, I'm trying to see if I can push it closer to 0.350 (with simply more training).

@jonilaserson
Copy link

Thanks for the update, Hans!

It should be the same, right? For models that use ResNet50 as backbone the new computation gives the same result as the previous computation. The difference would occur for models that play with the padding or strides differently.

@hgaiser
Copy link
Contributor Author

hgaiser commented Jul 24, 2019

Thanks for the update, Hans!

It should be the same, right? For models that use ResNet50 as backbone the new computation gives the same result as the previous computation. The difference would occur for models that play with the padding or strides differently.

Yeah it should be, but the 0.350 mAP score from before was basically luck. I never succeeded in reproducing that exact score ;) a score of 0.34 is fine, but there was only one snapshot that reached 0.350.

@martinzlocha
Copy link
Contributor

Is there a reason why you are not using seeds for training?

@hgaiser
Copy link
Contributor Author

hgaiser commented Jul 24, 2019

Very good question: no there's not :D

Too late now ;)

@martinzlocha
Copy link
Contributor

😢 Would it be worth creating a PR which allows users to set seeds using a parameter and by default they aren't set?

@hgaiser
Copy link
Contributor Author

hgaiser commented Jul 24, 2019

Yeah, I would think it can be useful, definitely.

@hgaiser
Copy link
Contributor Author

hgaiser commented Aug 1, 2019

I've started training from scratch, but after a while the training stopped improving (achieved around 0.27 mAP). I've restarted training multiple times (starting from the weights of the previous iteration) and have pushed the mAP to 0.292 so far.

This annoys me a bit because it suggests that the training procedure is not working properly (which I've known for a while now). Perhaps a different learning rate scheduler, or different optimizer, or both? I don't have time to look into these settings, so if someone wants to help then it is much appreciated.

Until then, I'll keep restarting training after it stagnated and see how far we get :)

@hgaiser
Copy link
Contributor Author

hgaiser commented Oct 10, 2019

For the record, this PR is still valid, but I haven't got the time to properly train a new model that gets similar accuracy as the current model.

@ZFTurbo
Copy link
Contributor

ZFTurbo commented Oct 31, 2019

Have a question about this PR. Could this PR potentially improve models accuracy after we additionally train them with correct data?

@hgaiser
Copy link
Contributor Author

hgaiser commented Oct 31, 2019 via email

@hsahin hsahin changed the base branch from master to main June 17, 2021 13:43
@hgaiser hgaiser closed this Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Computing the offset of anchors
5 participants