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

feat: integrate new loss and pooling options #664

Merged
merged 19 commits into from
Jan 30, 2023
Merged

Conversation

LMMilliken
Copy link
Contributor

@LMMilliken LMMilliken commented Jan 24, 2023

This pr adds support for the newly added loss and pooling options from core, as well as a new page in the documentation explaining how to use them.


  • This PR references an open issue
  • I have added a line about this change to CHANGELOG

@LMMilliken LMMilliken linked an issue Jan 24, 2023 that may be closed by this pull request
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added area/setup area/testing This issue/PR affects testing labels Jan 26, 2023
@LMMilliken LMMilliken marked this pull request as ready for review January 26, 2023 10:56
Copy link
Member

@bwanglzu bwanglzu left a comment

Choose a reason for hiding this comment

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

Great work! I like it!

docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
tests/unit/test_experiment.py Outdated Show resolved Hide resolved
@bwanglzu
Copy link
Member

i feel the name Advanced Methods is a bit too general (think about our section name is Advanced Topics), can we rename it to something else? Such as advanced losses, optimizers and pooling layers

Copy link
Member

@guenthermi guenthermi left a comment

Choose a reason for hiding this comment

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

Nice documentation, added some comments.

finetuner/__init__.py Outdated Show resolved Hide resolved
tests/unit/test_experiment.py Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
docs/advanced-topics/advanced-methods.md Outdated Show resolved Hide resolved
@LMMilliken LMMilliken force-pushed the feat-support-arcface branch 2 times, most recently from 1b10bb4 to 98c6d5c Compare January 26, 2023 14:39
@bwanglzu
Copy link
Member

the documentation is not included in the docs anymore, please double check

@LMMilliken
Copy link
Contributor Author

the documentation is not included in the docs anymore, please double check

Ah, I forgot to update the index when I renamed the file, fixed now

@guenthermi guenthermi mentioned this pull request Jan 27, 2023
2 tasks
Copy link
Member

@bwanglzu bwanglzu left a comment

Choose a reason for hiding this comment

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

LGTM! If the broken link is fixed.


SphereFace loss is a loss function that was first formulated for computer vision and face recognition tasks.
Finetuner supports two variations of this loss function, `ArcFaceLoss` and `CosFaceLoss`.
Instead of attempting to minimise the distance between positive pairs and maximise the distance between negative pairs, the SphereFace loss functions compare each sample with the center of each class
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Instead of attempting to minimise the distance between positive pairs and maximise the distance between negative pairs, the SphereFace loss functions compare each sample with the center of each class
Instead of attempting to minimise the distance between positive pairs and maximise the distance between negative pairs, the SphereFace loss functions compare each sample with the center of each class

I think here it is confusing what is the center of a class ?

In practice this is not really center but more estimated prototypes which is learn alongside the representation in the last linear softmax layer. Not sure how to put it in nice word so I let you the burden

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed it to '...with an estimate of the center point of each classes' embeddings.'
I don't want to go into too much detail explaining it but i think this is more accurate

Copy link
Member

@guenthermi guenthermi left a comment

Choose a reason for hiding this comment

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

Add a few more comments

Copy link
Member

@guenthermi guenthermi left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions
Copy link

📝 Docs are deployed on https://ft-feat-support-arcface--jina-docs.netlify.app 🎉

@LMMilliken LMMilliken merged commit 9348102 into main Jan 30, 2023
@LMMilliken LMMilliken deleted the feat-support-arcface branch January 30, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate new pooling and loss options into client
7 participants