Skip to content

add option to normalize embeddings #177

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

Merged
merged 2 commits into from
Nov 23, 2022

Conversation

PhilipMay
Copy link
Contributor

@PhilipMay PhilipMay commented Nov 13, 2022

Add option to normalize embeddings.

@PhilipMay PhilipMay changed the title Add option to normalize embeddings [WIP] Add option to normalize embeddings Nov 13, 2022
@PhilipMay
Copy link
Contributor Author

PhilipMay commented Nov 18, 2022

@blakechi do you have an idea how to best add this to the differentiable head ?

Somewhere here:

outputs = self.model_body(features)

Can we just change this:

                    outputs = self.model_body(features)

to

                    outputs = self.model_body.encode(features, normalize_embeddings=self.normalize_embeddings)

?

@blakechi
Copy link
Contributor

blakechi commented Nov 20, 2022

Hi @PhilipMay,

Sorry for the late reply.
I notice that sentence-transformers's encode function sets the model to eval mode at here.
Since users might want to train SetFitModel in the end-to-end fashion, I think it's not a good idea to use encode. Instead, we might need to normalize embeddings by ourself.

It's not totally the same case but I tried adding a LayerNorm between the body and head before. And I found that it degraded the performance. Just for reference. 😀

@PhilipMay
Copy link
Contributor Author

PhilipMay commented Nov 20, 2022

@blakechi what do you think about the norm. solution I added to this PR? Can you please check it?

See this commit: 88d7712

I just stole it form sbert. See:

https://github.com/UKPLab/sentence-transformers/blob/0b5ef4be93d2b21de3a918a084b48aab6ba48595/sentence_transformers/SentenceTransformer.py#L183-L184

Copy link
Contributor

@blakechi blakechi left a comment

Choose a reason for hiding this comment

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

It looks good to me :)

@blakechi
Copy link
Contributor

Just curious, have you tested the performance? Or do you have other purposes?

@PhilipMay
Copy link
Contributor Author

Just curious, have you tested the performance? Or do you have other purposes?

I tested performance with the "normal" Logistic Regression from SKlearn.
Normalizing reduces performance - which is surprising for me. Also see here:

#135 (comment)

@blakechi do you have an idea why?

PS: To test the diff. head is still on my todo list.

@PhilipMay PhilipMay changed the title [WIP] Add option to normalize embeddings Add option to normalize embeddings Nov 20, 2022
@PhilipMay
Copy link
Contributor Author

Can be merged from my point of view.

@blakechi
Copy link
Contributor

Just curious, have you tested the performance? Or do you have other purposes?

I tested performance with the "normal" Logistic Regression from SKlearn. Normalizing reduces performance - which is surprising for me. Also see here:

#135 (comment)

@blakechi do you have an idea why?

PS: To test the diff. head is still on my todo list.

I don't have a clear answer. But my guess is the magnitude of the embeddings might contains some information that helps classification, like the occurring frequency of each words?
An interesting report.

Btw, is it better to open embedding normalization to users only when we find merits from it?

@PhilipMay
Copy link
Contributor Author

Btw, is it better to open embedding normalization to users only when we find merits from it?

I would say that ML is a field where experimentation needs to be done. You never know what exactly helps and what not.
It also depends a lot on the use-case & data. So this enables ppl. to experiment. Since we disable it by default we do no harm. We just add one more option to experiment.

@PhilipMay PhilipMay changed the title Add option to normalize embeddings add option to normalize embeddings Nov 21, 2022
Copy link
Member

@lewtun lewtun left a comment

Choose a reason for hiding this comment

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

Thanks for adding this option @PhilipMay ! Since it doesn't interfere with the existing training, I'm happy to include it :)

@lewtun lewtun merged commit c5c8b6b into huggingface:main Nov 23, 2022
@PhilipMay PhilipMay deleted the add_norm_option branch November 23, 2022 13:43
@blakechi
Copy link
Contributor

I would say that ML is a field where experimentation needs to be done. You never know what exactly helps and what not. It also depends a lot on the use-case & data. So this enables ppl. to experiment. Since we disable it by default we do no harm. We just add one more option to experiment.

Good point, that's what I didn't notice and it makes sense to me!
Thanks for letting me know 😄 @PhilipMay

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.

3 participants