ExponentialML / ComfyUI_ELLA

ComfyUI Implementaion of ELLA: Equip Diffusion Models with LLM for Enhanced Semantic Alignment
Apache License 2.0
160 stars 10 forks source link

Fix ELLA timesteps #25

Open kijai opened 7 months ago

kijai commented 7 months ago

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 commented 7 months ago

Just tested it and this is so much better

Astropulse commented 7 months ago

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

JettHu commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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.