Skip to content
This repository was archived by the owner on Apr 27, 2024. It is now read-only.

Fix ELLA timesteps - #25

Open
kijai wants to merge 3 commits into
ExponentialML:mainfrom
kijai:main
Open

Fix ELLA timesteps#25
kijai wants to merge 3 commits into
ExponentialML:mainfrom
kijai:main

Conversation

@kijai

@kijai kijai commented Apr 17, 2024

Copy link
Copy Markdown

I have been comparing the results from this implementation to the diffusers implementation, and it's not on par. In diffusers ELLA is applied on each timestep, with the actual timestep value. Applying single sigma value in comfy doesn't produce the same results.

Now I don't know if this is a proper way to do it in Comfy, but it is giving far better results:
image

@FNSpd

FNSpd commented Apr 18, 2024

Copy link
Copy Markdown

Just tested it and this is so much better

@Astropulse

Copy link
Copy Markdown

Tested a ton, this makes ELLA behave as expected, thanks @kijai!

@JettHu

JettHu commented Apr 22, 2024

Copy link
Copy Markdown

Timestep-Aware Semantic Connector (TSC) is key contributions in our parper. It dynamically adapts semantics features over sampling time steps. This is also the reason why we opened an other ComfyUI-ELLA plugin repository.

But I haven’t noticed that comfyui has a conditioning timestep range parameter before. Can this part be merge into our official repo? We'll mention you in contributors.

@kijai

kijai commented Apr 22, 2024

Copy link
Copy Markdown
Author

Timestep-Aware Semantic Connector (TSC) is key contributions in our parper. It dynamically adapts semantics features over sampling time steps. This is also the reason why we opened an other ComfyUI-ELLA plugin repository.

But I haven’t noticed that comfyui has a conditioning timestep range parameter before. Can this part be merge into our official repo? We'll mention you in contributors.

If you think it's better, sure of course. I've used this way quite a bit now and seems to work good, but I'm not that experienced to know which way is better in the end.

@quixot1c

quixot1c commented Apr 22, 2024

Copy link
Copy Markdown

@JettHu There is the ConditioningSetTimeStepRange node in ComfyUI nodes.py, don't know if that will work for your type of conditioning:

class ConditioningSetTimestepRange:
    @classmethod
    def INPUT_TYPES(s):
        return {"required": {"conditioning": ("CONDITIONING", ),
                             "start": ("FLOAT", {"default": 0.0, "min": 0.0, "max": 1.0, "step": 0.001}),
                             "end": ("FLOAT", {"default": 1.0, "min": 0.0, "max": 1.0, "step": 0.001})
                             }}
    RETURN_TYPES = ("CONDITIONING",)
    FUNCTION = "set_range"

    CATEGORY = "advanced/conditioning"

    def set_range(self, conditioning, start, end):
        c = node_helpers.conditioning_set_values(conditioning, {"start_percent": start,
                                                                "end_percent": end})
        return (c, )

@JettHu

JettHu commented Apr 22, 2024

Copy link
Copy Markdown

If you think it's better, sure of course. I've used this way quite a bit now and seems to work good, but I'm not that experienced to know which way is better in the end.

I think it's more elegant than our current implementation. And it seems has better compatibility with the comfyui ecosystem.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants