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

refactoring and typing #25

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

refactoring and typing #25

wants to merge 3 commits into from

Conversation

noskill
Copy link
Collaborator

@noskill noskill commented Aug 11, 2023

No description provided.

@noskill noskill requested a review from CICS-Oleg August 11, 2023 15:40
@noskill
Copy link
Collaborator Author

noskill commented Aug 11, 2023

We can also remove "Pipe" from class names in pipes.py It is clear anyway that in module pipes we have some pipes

@noskill noskill requested review from mike-p-ni and removed request for mike-p-ni August 11, 2023 15:43
@noskill noskill changed the title minor refactoring and typing refactoring and typing Aug 14, 2023
@noskill noskill requested a review from fernflow August 14, 2023 12:34
@noskill
Copy link
Collaborator Author

noskill commented Aug 14, 2023

I removed Pipe from class names as redundant. It still very clear from Prompt2Im what class is doing without "Pipe".

other renames:
CIm2ImPipe -> ControlNet2Im
fimage -> img_path
fname -> img_path
csalem -> default_cs

@Necr0x0Der Necr0x0Der closed this Aug 14, 2023
@noskill noskill reopened this Aug 14, 2023
@Necr0x0Der
Copy link
Collaborator

I removed Pipe from class names as redundant. It still very clear from Prompt2Im what class is doing without "Pipe".

This is a weak argument to rename all the classes introducing breaking changes. In diffusers, many classes start with "StableDiffusion" and end with "Pipeline", although this also is obvious. Since different points of view on naming exist, and no approach is unconditionally supreme, introducing such breaking changes without necessity is simply a bad practice. It should also be added that other classes are envisioned in metafusion, which will not be Pipes, so it makes sense to distinguish them by naming convention.

default_cs

This is a bad name, because it is obvious that these are default values, but it is not obvious what "cs" is. If renaming is introduced, it should really be superior (and hopefully optimal).

ControlNet2Im

This is a bad name, since there is no sense in transforming ControlNet to Im. And why ControlNet is fully written, while Im is not? It is also important to make naming of all Pipe classes consistent, while this renaming contradicts to both Im2ImPipe and Cond2ImPipe.

Copy link
Collaborator

@Necr0x0Der Necr0x0Der left a comment

Choose a reason for hiding this comment

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

See comments above.

@noskill
Copy link
Collaborator Author

noskill commented Aug 15, 2023

default_cs is superior to cscalem, because what "calem" or "alem" even means?

Other variants I can suggest:
control_scales or just cscales - feels the same as gscale we use for guidance scale

@Necr0x0Der
Copy link
Collaborator

default_cs is superior to cscalem, because what "calem" or "alem" even means?

"scale" means scale. Thus, default_cs is not precisely superior. And it is also not the question if it is superior or not, but if you are doing such refactoring, you should be sure that it will not be done once again right away.

Other variants I can suggest: control_scales or just cscales - feels the same as gscale we use for guidance scale

I don't have strong preferences over cscales, default_control_scales, control_scales by themselves. The only thing that the naming style should be uniform at least within components or separate files of the library.

@noskill
Copy link
Collaborator Author

noskill commented Aug 17, 2023

"scale" means scale.

Yes, but what scalem means?

@Necr0x0Der
Copy link
Collaborator

Yes, but what scalem means?

I don't even remember. It is a temporary name.

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.

2 participants