Added CosFace Loss #198
Added CosFace Loss #198
Conversation
✅ Deploy Preview for capable-unicorn-d5e336 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
quaterion/loss/cos_face_loss.py
Outdated
| def l2_norm(inputs: torch.Tensor, dim: int = 0) -> torch.Tensor: | ||
| """Apply L2 normalization to tensor | ||
| Args: | ||
| inputs: Input tensor. | ||
| dim: Dimension to operate on. | ||
| Returns: | ||
| torch.Tensor: L2-normalized tensor | ||
| """ | ||
| outputs = inputs / torch.norm(inputs, 2, dim, True) | ||
|
|
||
| return outputs |
There was a problem hiding this comment.
Hi @crsdvaibhav, there is already a l2_norm function implemented for arcface_loss. If you are going to use l2_norm for cos_face_loss as well, then maybe it would be better to take l2_norm function to loss/utils/utils.py and call it from there in order to avoid code duplication.
There was a problem hiding this comment.
I was thinking the same, just needed a heads-up!
|
|
||
|
|
||
| class MultiObjectiveLoss(torch.nn.Module): | ||
| def __init__(self, losses: list, weights: Optional[list] = None): |
There was a problem hiding this comment.
I think, in order to improve expressivity, that would be better to include data type stored in the list such as weights: Optional[List[Tensor]].
There was a problem hiding this comment.
Sure, will do so. Thanks!
|
@KarahanS I have made the changes. Does this need some more changes before merge? |
Thanks for the updates! I'm not one of the official contributors of this project, so let's wait for the reviews by the maintainers for merging :) |
crsdvaibhav
changed the title
|
@monatis I have added the CosFace Loss and its test. Could you please review it? I have also added an implementation of Multi Objective Loss, could you please review it? |
| @@ -0,0 +1,22 @@ | |||
| from typing import Optional | |||
There was a problem hiding this comment.
This is a good start, but it has some issues:
- Not well documented with docstrings.
- Argument
groupsis missing in theforward()method. - Tests are missing.
After fixing these issue it might be a good start, but I'm thinking of something slightly different that will make GroupLoss and PairLoss compatible with one another through some utility functions. For example, I got slightly better results when I combined Triplet Loss and Multiple Negatives Ranking Loss, but it will require more experiments.
tl; dr: Let's remove MultiObjectiveLoss from this PR for now --it will be something more ambitious and definitely is worth its own PR 😁
There was a problem hiding this comment.
Sure! I will update the PR with the CosFace Loss only.
As for the issues with the Multi Objective Loss, I will surely solve them and put a new PR, and we can experiment with it there.
Thankyou!
There was a problem hiding this comment.
I have made the required changes:
- Removed the Multi Objective Loss
- Resolved the conflicts
crsdvaibhav
changed the title
|
I needed to close this. Could you please make another PR by rebasing from master and insuring that all the tests pass --because it didn't. |
@monatis This is still work in progress. Just wanted to ask few questions, by sharing the code.
Thankyou for the help!