-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
feat: Added support of Poly Loss #6457
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for early poke but a few thoughts @frgfm
@frgfm Just wanted to check in and ensure you are not blocked or need any assistance. Let me know if you do. :) |
Hello there :) My bad, I just caught up with the PR! I left it as a draft because the missing part is the unit test: I saw that the ones for the focal loss are quite extensive. Do I need to be that thorough? |
@frgfm Ah yes the unit-tests. I would recommend following the pattern that we have on existing tests and have a similar coverage. Hopefully following the existing infra will make your work easier. The reason we really need them is because previously we spotted a few nasty bugs on some of the losses. We are only couple of weeks prior cutting the branch for the upcoming release so if we want to include this feature we need to have elevated degree of confidence. |
As discussed in #6439, this PR adds support for the poly loss. A few things are left to decide / do:
Closes #6439
cc @datumbox