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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Additive Margin Softmax #1125

Closed
3 of 4 tasks
Atharva-Phatak opened this issue Mar 20, 2021 · 10 comments
Closed
3 of 4 tasks

[Feature] Additive Margin Softmax #1125

Atharva-Phatak opened this issue Mar 20, 2021 · 10 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@Atharva-Phatak
Copy link
Contributor

Atharva-Phatak commented Mar 20, 2021

馃殌 Feature Request

I would like to add Additive Margin Softmax to its amazing list of various 'Face' losses.

Motivation

Catalyst has nice support for metric learning and I would like catalyst modules to be expanded more and possibly have most SOTA implementations of Face metrics.

Proposal

Just like how ArcFace and CosFace are implemented in the catalyst library, I would like to add AM-Softmax to this list as well. Possibly other metric learning modules if provided the opportunity.

Additional context

Additive margin softmax was used in 2nd place solution of a Kaggle competition.

Checklist

Note

This goes without saying I want to implement it

@Atharva-Phatak Atharva-Phatak added enhancement New feature or request help wanted Extra attention is needed labels Mar 20, 2021
@Scitator
Copy link
Member

Hi,

the idea looks great, especially if you are interested in the implementation contribution :)

I also think @ditwoo would be interested in too.

@Atharva-Phatak
Copy link
Contributor Author

Hi, yes I have a working implementation ready, I can write up some tests for it and create a pr.

@Scitator
Copy link
Member

That's would be great,
btw, you could check current tests for the "Faces" family here

@Atharva-Phatak
Copy link
Contributor Author

I will take look at it for sure.

@Atharva-Phatak
Copy link
Contributor Author

@Scitator I took a look at the tests, the tests seems to be well designed. So no worries. I will start with implementation and create PR soon.

@Atharva-Phatak
Copy link
Contributor Author

@Scitator can you give me some tips on how to pass the codestyle ?
Here are the steps I followed.

  1. I forked the latest repo and cloned it
  2. installed the dependencies for requirements.txt and requirements-dev.txt
  3. I run catalyst-make-codestyle first and the catalyst-check-codestyle
  4. Files seem to be reformatted but flake8 errors persist. Also, there are no errors in the file which I added.(Last time also the same thing happened with codestyle, hence I want to do it perfectly this time.)

Hence I wanted some help to make codestyle do not throw any error.

@Scitator
Copy link
Member

sorry, but could not reproduce the issue: https://colab.research.google.com/drive/1JCGTVvWlrIsLXMPRRRSWiAstSLic4nbA?usp=sharing

@Atharva-Phatak
Copy link
Contributor Author

Atharva-Phatak commented Mar 22, 2021

@Scitator Thank you now my codestyle is compatible. But when I run tests using pytest .. I get the following error
ImportError: cannot import name 'SummaryWriter' from 'tensorboardX' (unknown location)

Here are the specs of my env
platform linux 20.04 -- Python 3.8.5, pytest-5.3.1, py-1.10.0, pluggy-0.13.1

Any tips to resolve it ?

@Scitator
Copy link
Member

I think, you still have to reinstall the requirements TensorboardX is must-have one :)
https://github.com/catalyst-team/catalyst/blob/master/requirements/requirements.txt#L13

@Scitator
Copy link
Member

looks like merged ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants