Skip to content

Conversation

chingisooinar
Copy link
Contributor

Hello,
I am implementing Sub-center Arcface loss for this project according to the following paper
https://www.ecva.net/papers/eccv_2020/papers_ECCV/papers/123560715.pdf
I also added a notebook as an example with MNIST dataset. I am planning to implement a function to clean a dataset using dominant centers as stated in the paper. However, I am not sure what would be the best way to do it. If you would like to see Sub-center arcface in this repo as well, let me know what you think please. If there's anything you want me to change, please let me know as well!

@KevinMusgrave
Copy link
Owner

Thanks for starting this! I think sub-center arcface would be a great addition to the library. Is your implementation complete?

@chingisooinar
Copy link
Contributor Author

@KevinMusgrave not yet, I believe I need to implement a function that would return dominant centers (our of subcenters) for data cleaning as stated in the paper. I am thinking of how I can integrate it neatly. I will try to finish it soon!

@samils7
Copy link

samils7 commented Feb 17, 2022

Hi Chingis, thanks for your contribution on implementing Sub-center Arcface loss.
I also want to try Sub-center Arcface loss and this implementation will help me a lot. I am willing to help if there is incomplete implementation you are not already working on.

Additionally, what do you think about adding dynamic margins for ArcFace loss as introduced in: Google Landmark Recognition 2020 Competition Third Place Solution
and explained in: ArcFace with Dynamic Margin

@KevinMusgrave
Copy link
Owner

@samils7 Thanks for bringing up dynamic margin. It looks interesting and straightforward to implement. I've created an issue for this feature.

@chingisooinar
Copy link
Contributor Author

@KevinMusgrave Hello, I tried to implement a method to predict outliers (noise) and dominant centers and integrate it neatly with your library. Sub center Arcface is quite unique, so please take a look at my implementation and an example notebook with MNIST dataset. Let me know if there is anything I can change and improve for you!

@chingisooinar
Copy link
Contributor Author

@samils7 thank you. I think it's almost done. I might just need to do some cleaning/refactoring. Feel free to share your ideas regarding my implementation.

@KevinMusgrave
Copy link
Owner

KevinMusgrave commented Feb 19, 2022

Thanks @chingisooinar! Can you add a test for this new loss function?

  1. Create the file test_subcenter_arcface_loss.py in the tests/losses folder.
  2. Write some simple tests. For example, make sure computing the loss works. Make sure get_outliers works as expected.

In case you're unfamiliar with the unittest library, here's a small example:

class TestCommonFunctions(unittest.TestCase):
def test_torch_standard_scaler(self):
torch.manual_seed(56987)
embeddings = torch.randn(1024, 512)
scaled = c_f.torch_standard_scaler(embeddings)
true_scaled = StandardScaler().fit_transform(embeddings.cpu().numpy())
true_scaled = torch.from_numpy(true_scaled)
self.assertTrue(torch.all(torch.isclose(scaled, true_scaled, rtol=1e-2)))

You can run your test at the command line:

python -m unittest tests/losses/test_subcenter_arcface_loss.py

@chingisooinar
Copy link
Contributor Author

chingisooinar commented Feb 22, 2022

@KevinMusgrave hello I am sorry but I am having some troubles with relative imports in python scripts.
ImportError: attempted relative import with no known parent package
Do you know how I can fix this?

@KevinMusgrave
Copy link
Owner

KevinMusgrave commented Feb 22, 2022

I run the following command while in the root folder, and it works:

python -m unittest tests/losses/test_subcenter_arcface_loss.py

(I get a different import error, which is that ArcFaceLoss isn't in large_margin_softmax_loss.py.)

@chingisooinar
Copy link
Contributor Author

@KevinMusgrave thank you for your hints. I implemented some unit tests and fixed my mistakes. Please take a look and let me know if there's anything I can do for you!

@KevinMusgrave KevinMusgrave changed the base branch from master to dev February 28, 2022 23:12
@KevinMusgrave KevinMusgrave merged commit ef58dd9 into KevinMusgrave:dev Mar 1, 2022
@KevinMusgrave
Copy link
Owner

Thanks @chingisooinar! The only major change I made was that I removed the "normalize" option from get_outliers, and I made the returned dominant centers unnormalized instead of normalized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants