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

[refactor embeddings] gligen + ip-adapter #6244

Merged
merged 7 commits into from
Dec 28, 2023
Merged

Conversation

yiyixuxu
Copy link
Collaborator

continue refactoring embeddings: GLIGEN + IP-Adapter

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.



class PositionNet(nn.Module):
class GLIGENTextBoundingboxProjection(nn.Module):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@patrickvonplaten
Any rules around when we call it "Projection" vs "Embedding"?

Copy link
Member

Choose a reason for hiding this comment

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

I follow this rule of thumb.

When you're representing some feature (time, bbox coordinates, tokens, etc.) in the latent space for the first time, it's better to call those representations embeddings. The subsequent operations performed on those representations are projections (more aligned with the literature in linear algebra).

Copy link
Contributor

Choose a reason for hiding this comment

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

I like GLIGENTextBoundingboxProjection - it's nicely specific

@yiyixuxu
Copy link
Collaborator Author

The remote_code tests fail due to the class name change. What should we do about this?

@sayakpaul
Copy link
Member

The remote_code tests fail due to the class name change. What should we do about this?

  1. We could define a mapping at the import level and manually check for them.
  2. Introduce this as a breaking change.

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

Very nice refactor - good to have more specific names here

@DN6
Copy link
Collaborator

DN6 commented Dec 27, 2023

@yiyixuxu I think it's okay to introduce as a breaking change. I don't seem to see it imported directly in a lot of public repos
https://github.com/search?q=%22from+diffusers.models.embeddings+import+PositionNet%22+language:Python&type=code

And importing the GLIGEN pipeline will be unaffected.

Copy link
Collaborator

@DN6 DN6 left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@yiyixuxu yiyixuxu merged commit 4c483de into main Dec 28, 2023
15 of 16 checks passed
@yiyixuxu yiyixuxu deleted the refactor-embedding-rest branch December 28, 2023 04:48
donhardman pushed a commit to donhardman/diffusers that referenced this pull request Dec 29, 2023
* refactor ip-adapter-imageproj, gligen

---------

Co-authored-by: yiyixuxu <yixu310@gmail,com>
antoine-scenario pushed a commit to antoine-scenario/diffusers that referenced this pull request Jan 2, 2024
* refactor ip-adapter-imageproj, gligen

---------

Co-authored-by: yiyixuxu <yixu310@gmail,com>
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
* refactor ip-adapter-imageproj, gligen

---------

Co-authored-by: yiyixuxu <yixu310@gmail,com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants