Rename variables from single letter to better naming

[patrickvonplaten]

Pretty much all code in https://github.com/huggingface/diffusers/blob/main/src/diffusers/models/resnet.py still corresponds to the copy-pasted version from the beginning.

While weight names shouldn't be renamed, we should rename all single letter variables to more explicit variable naming. E.g. h -> hidden_states.

Single letter variables are hard to deal with as search and replace doesn't work on them and are hard to read

[patrickvonplaten]

Also maybe @anton-l if you'd like

[patrickvonplaten]

That would be great @antival3nt :heart_eyes:

[rashmimarganiatgithub]

@patrickvonplaten , please assign to me.

[patrickvonplaten]

Feel free to take it @rashmimarganiatgithub - all yours! Generally as soon as someone opens a PR for an issue, we try to work together on this PR

[daspartho]

Hi @patrickvonplaten I'd be happy to work on any other variable names that need to be renamed.

[patil-suraj]

Hey @daspartho ! Awesome, there are some single letter variables in attention.py file, feel free to work on those if you want. I'm listing the places that needs change below.

https://github.com/huggingface/diffusers/blob/b2b3b1a8ab83b020ecaf32f45de3ef23644331cf/src/diffusers/models/attention.py#L140 https://github.com/huggingface/diffusers/blob/b2b3b1a8ab83b020ecaf32f45de3ef23644331cf/src/diffusers/models/attention.py#L195 https://github.com/huggingface/diffusers/blob/b2b3b1a8ab83b020ecaf32f45de3ef23644331cf/src/diffusers/models/attention.py#L313 https://github.com/huggingface/diffusers/blob/b2b3b1a8ab83b020ecaf32f45de3ef23644331cf/src/diffusers/models/attention.py#L331

[daspartho]

Hi @patil-suraj Thank you for pointing out the necessary changes; I'll work on it. I'm just not sure what the exact variable name for the single-letter variables should be. Can you please help me? Thanks :)

[rashmimarganiatgithub]

@daspartho ,q, k, v can be replaced with query,key,value. b, c, h, w can be replaced with batch,channel,height,weight .

[daspartho]

Thank you for clarifying, @rashmimarganiatgithub. I'll start a PR soon.

[daspartho]

I've opened a PR for the issue :) #449

[daspartho]

Hi @patil-suraj I'd be happy to work on any additional files that require changes.

[i-am-epic]

Hi @patil-suraj , @patrickvonplaten I would like to help with other files for better variable name changes.

[patil-suraj]

Hey @i-am-epic @daspartho , there are still some places with single letter variables in resenet.py , feel free to work on those! I'm listing them down here.

https://github.com/huggingface/diffusers/blob/main/src/diffusers/models/resnet.py#L37 https://github.com/huggingface/diffusers/blob/main/src/diffusers/models/resnet.py#L54 https://github.com/huggingface/diffusers/blob/main/src/diffusers/models/resnet.py#L177 https://github.com/huggingface/diffusers/blob/main/src/diffusers/models/resnet.py#L239 https://github.com/huggingface/diffusers/blob/main/src/diffusers/models/resnet.py#L329

[i-am-epic]

Hi @patil-suraj, Thank you for pointing out the changes; I'll work on it. I'm not sure my naming analogy is perfect. correct me with better naming convention, x as parameter in forward function will be input_tensor and it returns output_tensor (and based on upsample, downsample or padding x can be upsample_input,downsample_input, padded_input).

[daspartho]

Opened a PR for the issue :) #676 I hope @i-am-epic doesn't mind me jumping in.

[i-am-epic]

@daspartho no issues it would had been my first contribution.

[daspartho]

Hi! @i-am-epic, You could work on this one. Happy first contribution :) https://github.com/huggingface/diffusers/blob/f10576ad5c9dbd17c59e0af12a26583e3a540e20/src/diffusers/models/resnet.py#L329

[i-am-epic]

Hi! @i-am-epic, You could work on this one. Happy first contribution :)

https://github.com/huggingface/diffusers/blob/f10576ad5c9dbd17c59e0af12a26583e3a540e20/src/diffusers/models/resnet.py#L329 @daspartho can u help me what's wrong in my pull request #677 thanks :)

[i-am-epic]

Opened new PR #677 Please do verify, thanks.

[dgarnitz]

Is this still open? I see there is a merged PR above. I can jump on it if it is still open

[SauravMaheshkar]

I raised https://github.com/huggingface/diffusers/pull/3868, fixing some of the last few remaining bits. Any ideas for appropriate names for the Temporal blocks viz. the variable name t ?