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

feat: add multiple classes conditionning #628

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Bycob
Copy link
Collaborator

@Bycob Bycob commented Mar 5, 2024

@beniz This PR is not ready, I have several questions about the integration of multiple class in joligen:

  • The cls_semantic_nclasses seems to be used only by GAN models, for diffusion, class conditioning uses the f_s_semantic_nclasses parameter. Should we use cls_semantic_nclasses instead? cc @royale
  • Is it fine to have multiple heads to embed the class, like I did? I'm not satisfied of this solution because it adds complexity to the code, I kept single head for backward compatibility with old models
  • Check the backward compatibility with train_config, since *_semantic_nclasses is now an array instead of an int
  • (As @royale suggested, we could sum embeddings of different classes instead of concatening them)

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.

1 participant