Add embedding projector for encoder and decoder #247
Conversation
|
Hi @stephantul! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
| @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - Expose bias flag for feedforwards, same default as Timm [#220] | |||
| - Update eps value for layernormm, same default as torch [#221] | |||
| - PreNorm bugfix, only one input was normalized [#233] | |||
| - Fix bug where embedding dimensions that did not match model dim would lead to a crash [#244] | |||
There was a problem hiding this comment.
this would need to be moved up, it would be part of the next release ? I'm kind of on the fence as of whether this is a "bug" (there are plenty of settings which you can pass which will "crash", in that they will tell you down the line that the dimensions mismatch), but as you see fit
There was a problem hiding this comment.
Ah yeah, not a bug, sorry! Feel free to rename it as you see fit.
|
looks good to me except for minor remarks, thanks a lot @stephantul ! I can fix this and the linting if you'd like, else feel free to update the PR anytime |
|
now mypy :] it helps installing pre-commit, we should explain it a bit better, more info here. Thanks already, getting there ! |
|
(doc build broke at some point, rebasing on master will fix that, sorry about that @stephantul ) |
|
@stephantul see https://github.com/stephantul/xformers/pull/1 for a proposed fix |
lintfix suggestion to be able to #247
|
Thanks for the fix! I was offline during the weekend, and was planning to fix it today, but I'm glad you took care of it 😄 |
Codecov Report
@@ Coverage Diff @@
## main #247 +/- ##
==========================================
- Coverage 92.21% 92.07% -0.14%
==========================================
Files 60 60
Lines 3312 3331 +19
==========================================
+ Hits 3054 3067 +13
- Misses 258 264 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
LGTM @stephantul, thank you ! One last thing would be to unit test this case, it can be just a matter of adding another case here. I can do that later on if you prefer, I understand that there are a lot of steps to contributing and wouldn't want to wear you down :) |
There was a problem hiding this comment.
LGTM, as you prefer unit test wise, I can follow up on that ! Thanks a bunch @stephantul
|
landing that, adding more unit tests in a seperate PR, easy follow up |
What does this PR do?
Fixes #244 .
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.