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.

Sign up for GitHub

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

Jump to bottom

add classifier_dropout to classification heads #12794

Merged
merged 13 commits into from
Jul 26, 2021
Merged

add classifier_dropout to classification heads #12794

merged 13 commits into from
Jul 26, 2021

Conversation

PhilipMay
Copy link
Contributor

@PhilipMay PhilipMay commented Jul 19, 2021 •
edited
Loading

PR for #12781 and maybe a fix for #12792

Logbook

  • added to Electra: SequenceClassification and TokenClassification
  • added to BERT: SequenceClassification and TokenClassification
  • added to RoBERTa: SequenceClassification and TokenClassification
  • added to big_bird: SequenceClassification and TokenClassification
  • added to mobilebert: SequenceClassification and TokenClassification
  • added to reformer: SequenceClassification and TokenClassification
  • added to ConvBERT: SequenceClassification and TokenClassification
  • added to albert: SequenceClassification and TokenClassification

@PhilipMay PhilipMay mentioned this pull request Jul 19, 2021
Closed
@PhilipMay PhilipMay changed the title add classifier_dropout to classification heads [WIP] add classifier_dropout to classification heads Jul 19, 2021
sgugger
sgugger approved these changes Jul 19, 2021
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

This can be added to the other models too!

src/transformers/models/electra/configuration_electra.py Outdated Show resolved Hide resolved
src/transformers/models/electra/configuration_electra.py Outdated Show resolved Hide resolved
@sgugger sgugger requested a review from LysandreJik July 19, 2021 17:48
@PhilipMay PhilipMay force-pushed the add_head_dropout branch 3 times, most recently from cc8d91e to 4714546 Compare July 19, 2021 18:40
@PhilipMay @sgugger
no type annotations yet
681d7f0 
Co-authored-by: Sylvain Gugger <[email protected]>
@PhilipMay
Copy link
Contributor Author

@sgugger should classifier_dropout be only added for <model_name>ForSequenceClassification
or also to <model_name>ForTokenClassification?

@PhilipMay PhilipMay force-pushed the add_head_dropout branch 2 times, most recently from 250a952 to 4714546 Compare July 19, 2021 19:10
@sgugger
Copy link
Collaborator

sgugger commented Jul 19, 2021

It should be used for both, yes.

LysandreJik
LysandreJik approved these changes Jul 20, 2021
View reviewed changes
Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

LGTM

@PhilipMay
Copy link
Contributor Author

This is still WIP - I want to add more models. Please do not merge yet ...

@PhilipMay PhilipMay mentioned this pull request Jul 21, 2021
Closed
@PhilipMay PhilipMay changed the title [WIP] add classifier_dropout to classification heads add classifier_dropout to classification heads Jul 22, 2021
@PhilipMay
Copy link
Contributor Author

I am done with this PR. It is ready for review.
See logbook above for changed model types.

@PhilipMay PhilipMay requested review from LysandreJik and sgugger July 22, 2021 19:38
JetRunner
JetRunner approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@JetRunner JetRunner left a comment

Choose a reason for hiding this comment

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

Never mind :)

sgugger
sgugger approved these changes Jul 26, 2021
View reviewed changes
Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Thanks again for working on this!

@sgugger sgugger merged commit 0c1c42c into huggingface:master Jul 26, 2021
@PhilipMay PhilipMay mentioned this pull request Jan 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgugger sgugger sgugger approved these changes

@patrickvonplaten patrickvonplaten patrickvonplaten approved these changes

@JetRunner JetRunner JetRunner approved these changes

@LysandreJik LysandreJik Awaiting requested review from LysandreJik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@PhilipMay @sgugger @JetRunner @patrickvonplaten @LysandreJik