Implementation of Focal loss

[shikhargoswami]

Focal loss was introduced in the RetinaNet paper (https://arxiv.org/pdf/1708.02002.pdf).

Focal loss is useful for classification when you we highly imbalanced classes. It down-weights well-classified examples and focuses on hard examples. The loss value is much high for a sample which is misclassified by the classifier as compared to the loss value corresponding to a well-classified example.

Used in single-shot object detection where the imbalance between the background class and other classes is extremely high.

Here's it's tensorflow implementation (https://github.com/tensorflow/addons/blob/v0.12.0/tensorflow_addons/losses/focal_loss.py#L26-L81)

PR Checklist

• Tests are added
• Entry in NEWS.md
• Documentation, if applicable
• Final review from @dhairyagandhi96 (for API changes).

[darsnack]

Thanks for this contribution! Left some small comments, but the approach looks good to me.

[DhairyaLGandhi]

Rather than loop iteration, it might be faster to do array operations. That might be faster for when you actually have to materialize the result with bigger arrays

[CarloLucibello]

That paper is cited enough that it may be worth having this loss in flux, although pytorch doesn't have it and tensorflow has it only as an addon

[darsnack]

Yeah having never heard of this loss before, I checked the paper. It's well-cited, so good enough for me to include in Flux.

[shikhargoswami]

@darsnack @CarloLucibello Need help on converting the list compreshension to array operations. Any useful leads?

[darsnack]

@shikhargoswami You might take a look at how binarycrossentropy is written to see how to address @DhairyaLGandhi's comment. The xlogy utility is what you need I believe.

I didn't see a performance difference when testing, but I didn't use large array sizes. Even if there isn't a gap in performance, it would be good to use the same style as the other loss functions.

[CarloLucibello]

assuming 0/1 you can write

p_t = y .* ŷ  + (1 .- y) .* (1 .- ŷ)  

So this is used only for binary classification? This should be mentioned

[shikhargoswami]

I guess I only implemented Binary classification. Here are the changes made:

• List comprehension -> Array operation
• Implemented categorical_focal_loss and added it's test
• Minor changes in docstring

Check if it needs any other change

[darsnack]

Personally, I would prefer the names focal_loss/binary_focal_loss to match crossentropy/binarycrossentropy.

[darsnack]

Some suggestions on the docstrings.

[darsnack]

I would commit the changes that address the comments above. I think this is close to being ready for approval, but we'll need to come to a consensus on the numerical stability vs. performance issue. That will require input for other maintainers, so probably we won't be able to merge today.

[DhairyaLGandhi]

I'll also need to review the api before we make a final call

[darsnack]

This looks good to go for me, but @DhairyaLGandhi will need to approve the final API.

[DhairyaLGandhi]

Thanks, I've added a few last thoughts, but this is looking good. We might want to review how we write our losses but that would be a more general shift, so keeping it consistent with the current seems sensible to me. Thanks again for the good work @shikhargoswami

[CarloLucibello]

I think it would be better to assume logits as inputs instead of probabilities. In model-zoo we use logitcrossentropy everyqhere instead of crossentropy for numerical stability

[darsnack]

This goes back to the numerical stability question. We would need to rework the implementation to make the logits version stable. If we do switch to logits, maybe the naming should be logitfocalloss and logitbinaryfocalloss. It's tough on the eyes but consistent with the cross entropy convention.

[CarloLucibello]

the fact that we have both crossentropy and logitcrossentropy is quite unfortunate, when I revisited the loss functions I thought about having a single definition function crossentropy(yhat, y; logits=true) but couldn't figure out a nice deprecation path. Working with unnormalized log-probabilities is what one should want in most cases for numerical stability. Should we experiment with this keyword arg approach here? Or we can just leave this as it is, not worth too much overthinking

[darsnack]

I like the possibility of making "logit" a kwarg, but I think it is better in a separate PR. I think for here, let's just decide which version (logit or not) we want, and we can keep it consistent with what we already have.

[DhairyaLGandhi]

I think the less magical the function, the better, and this makes certain contracts very implicit, like which function softmax we use. It goes against the self documenting code we expect from Flux. I'd need to see some very compelling reasons to do opaque looking keywords in Flux.

It's one reason I feel like we should remove the agg, it doesn't justify being in every loss function when it clearly doesn't add much to most of them.

[CarloLucibello]

let's keep it as it is then, it's already done and logitbinaryfocalloss is quite horrifying 😄

[DhairyaLGandhi]

I agree the names... Need work

[darsnack]

@shikhargoswami that should fix it. Can you address the remaining comments?

[darsnack]

Let's wait for CI to pass then I think this can merge.

[shikhargoswami]

@darsnack Thanks a lot for the help!

[darsnack]

No problem! Thank you for your contribution!

[darsnack]

bors r+

[darsnack]

bors r-

[darsnack]

bors r-

[bors]

Canceled.

[darsnack]

@shikhargoswami sorry to delay, but I realized you didn't add the focal loss docstring to the actual docs in docs/models/losses.md.

[darsnack]

Thanks!

[darsnack]

bors r+

[bors]

Build succeeded:

• buildkite/flux-dot-jl

[DhairyaLGandhi]

Kyle, for the future, this is an API related change and the process does require a final approval from me.

And good job @shikhargoswami !

[darsnack]

Sorry, my mistake, I thought you had approved of the API. Next time I will leave the final merge to you on API changes.