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

List inputs in lda encode/decode, and clip text_encoder. #213

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

piercus
Copy link
Collaborator

@piercus piercus commented Jan 27, 2024

Naming of lda.decode_latents(latents) feels strange cause lda.decode(latents) is also decoding latents.

I suggest using lda.decode_image/lda.decode_images and lda.encode_image/lda.encode_images

To not break examples, the current PR is keeping an alias lda.decode_latents(latents) -> lda.decode_image(latents)

@piercus
Copy link
Collaborator Author

piercus commented Jan 29, 2024

@deltheil @limiteinductive

In current code latents and latent are both used for a single Tensor.
I want to suggest to switch every single tensor variable to singular latent and use latents only when there is a sequence.

The naming for helpers would be image_to_latent and images_to_latent

WDYT ?

@deltheil
Copy link
Member

In current code latents and latent are both used for a single Tensor.
I want to suggest to switch every single tensor variable to singular latent and use latents only when there is a sequence.

Not sure using plural will mean the same depending on the reader (e.g. latents -> latent variables vs. sequence of latent vectors). Perhaps we could make our life simpler and just adopt "latents" systematically (seems pretty widespread already: "CLIP latents", etc).

@limiteinductive any strong preference on your end?

@limiteinductive
Copy link
Contributor

@limiteinductive, is there any strong preference on your end?

It's almost a convention that "latent" is always plural. Since a Tensor is a batch, you can have a single Tensor is still have multiple "latents."

@piercus
Copy link
Collaborator Author

piercus commented Jan 31, 2024

Thanks for your help @deltheil @limiteinductive
Please review this new version.

@piercus piercus requested a review from deltheil January 31, 2024 21:36
Copy link
Member

@deltheil deltheil left a comment

Choose a reason for hiding this comment

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

Thanks! Only minor feedback, please take a look

src/refiners/fluxion/utils.py Outdated Show resolved Hide resolved
src/refiners/fluxion/utils.py Outdated Show resolved Hide resolved
src/refiners/foundationals/clip/tokenizer.py Outdated Show resolved Hide resolved
tests/e2e/test_diffusion.py Outdated Show resolved Hide resolved
tests/foundationals/clip/test_text_encoder.py Show resolved Hide resolved
@deltheil deltheil added run-ci Run CI and removed run-ci Run CI labels Feb 1, 2024
deltheil
deltheil previously approved these changes Feb 1, 2024
@deltheil
Copy link
Member

deltheil commented Feb 1, 2024

@piercus do you mind rebasing this branch onto main and solve the conflicts? (ideally, you should also squash into a single commit - but I can do it myself at the end if that's simpler).

@piercus piercus force-pushed the batch-encode branch 2 times, most recently from 51544cb to 21d3a23 Compare February 1, 2024 14:06
@piercus
Copy link
Collaborator Author

piercus commented Feb 1, 2024

Squash and merge done

@deltheil deltheil added run-ci Run CI and removed run-ci Run CI labels Feb 1, 2024
* text_encoder([str1, str2])
* lda decode_latents/encode_image image_to_latent/latent_to_image
* images_to_tensor, tensor_to_images
---------
Co-authored-by: Cédric Deltheil <[email protected]>
@deltheil deltheil added run-ci Run CI and removed run-ci Run CI labels Feb 1, 2024
@deltheil deltheil merged commit 4a6146b into finegrain-ai:main Feb 1, 2024
1 check passed
deltheil added a commit that referenced this pull request Feb 2, 2024
`encode_image` has been deprecated part of #213
deltheil added a commit that referenced this pull request Feb 2, 2024
`encode_image` has been deprecated part of #213
deltheil added a commit that referenced this pull request Feb 2, 2024
`encode_image` has been deprecated part of #213
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-ci Run CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants