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] add focal_loss and label_smoothing_loss #121

Closed

Conversation

rohitgr7
Copy link
Contributor

@rohitgr7 rohitgr7 commented Jul 18, 2020

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Added FocalLoss and LabelSmoothingLoss as discussed on slack.
TODO:

  1. make changes if requested
  2. update docs
  3. Add tests

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@pep8speaks
Copy link

pep8speaks commented Jul 18, 2020

Hello @rohitgr7! Thanks for updating this PR.

Line 50:121: E501 line too long (125 > 120 characters)
Line 82:121: E501 line too long (128 > 120 characters)
Line 102:1: W293 blank line contains whitespace
Line 143:1: W293 blank line contains whitespace
Line 151:121: E501 line too long (123 > 120 characters)
Line 162:5: E301 expected 1 blank line, found 0
Line 166:1: E302 expected 2 blank lines, found 1
Line 176:121: E501 line too long (125 > 120 characters)

Comment last updated at 2020-07-22 22:42:20 UTC

@mergify mergify bot requested a review from Borda July 18, 2020 20:40
@rohitgr7 rohitgr7 force-pushed the add_classification_losses branch from 2ef6b76 to 9a3bd0f Compare July 18, 2020 20:46
@codecov
Copy link

codecov bot commented Jul 18, 2020

Codecov Report

Merging #121 into master will decrease coverage by 1.65%.
The diff coverage is 28.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   92.25%   90.60%   -1.66%     
==========================================
  Files          79       80       +1     
  Lines        4031     4139     +108     
==========================================
+ Hits         3719     3750      +31     
- Misses        312      389      +77     
Flag Coverage Δ
#unittests 90.60% <28.70%> (-1.66%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pl_bolts/losses/classification.py 28.70% <28.70%> (ø)

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 9ef90b4...369db1a. Read the comment docs.

@rohitgr7
Copy link
Contributor Author

cc @justusschock

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

pls add Documentation :]

@justusschock
Copy link
Member

@rohitgr7 can you maybe also add a functional interface?

when you look at https://github.com/justusschock/dl-utils/blob/master/dlutils/losses/focal.py you can find a more detailed interface, that also works on binary/logits. Could we maybe adopt something similar here? I'm afraid I haven't tested the implementation there. The reduce and onehot functions should be available in pytorch-lightning.metrics

@rohitgr7
Copy link
Contributor Author

@justusschock Made the changes locally with the new interface. Any resources from where I can test my implementation. All I can find online is just the implementation of binary-version which I tested and working fine. The problem is with multi-class version one. Can't find anything for that.

@rohitgr7 rohitgr7 changed the title add focal_loss and label_smoothing_loss [WIP] add focal_loss and label_smoothing_loss Jul 22, 2020
@rohitgr7
Copy link
Contributor Author

Mind the docs as of now. Tested BinaryFocalLoss \ WithLogits and LabelSmoothingLoss. Added weight and ignore_index parameters to match with pytorch loss api. Can't find multi-class version of focal loss to test it. Also with weight parameter pytorch uses weighted mean when reduction='mean', but pl_metrics reduce function takes elementwise_mean which is just the basic mean. Not sure if I should keep weight parameter here.

@Borda Borda added the enhancement New feature or request label Aug 5, 2020
@rohitgr7 rohitgr7 mentioned this pull request Sep 27, 2020
8 tasks
@stale
Copy link

stale bot commented Oct 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the won't fix This will not be worked on label Oct 4, 2020
@rohitgr7 rohitgr7 removed the won't fix This will not be worked on label Oct 4, 2020
@stale
Copy link

stale bot commented Dec 3, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the won't fix This will not be worked on label Dec 3, 2020
@rohitgr7 rohitgr7 removed the won't fix This will not be worked on label Dec 4, 2020
@Borda
Copy link
Member

Borda commented Jan 2, 2021

@rohitgr7 how is it going here, is it still WIP? 🐰

@Borda Borda requested a review from akihironitta January 2, 2021 21:25
@rohitgr7
Copy link
Contributor Author

rohitgr7 commented Jan 3, 2021

totally forgot about this one. will get this done next week.

@Borda
Copy link
Member

Borda commented Jan 18, 2021

@rohitgr7 any update here? 👼

@rohitgr7 rohitgr7 closed this Mar 6, 2021
@rohitgr7 rohitgr7 deleted the add_classification_losses branch March 6, 2021 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants