Skip to content

Rename variables from single letter to better naming #198

Closed
#3868
Closed
Rename variables from single letter to better naming#198
#3868
Labels
enhancementNew feature or requestgood first issueGood for newcomers

Description

@patrickvonplaten
patrickvonplaten
opened on Aug 17, 2022

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

Activity

  1. patrickvonplaten
    added
    enhancementNew feature or request
    on Aug 17, 2022

  2. patrickvonplaten commented on Aug 17, 2022

    @patrickvonplaten
    patrickvonplaten
    on Aug 17, 2022
    ContributorAuthor

    Also maybe @anton-l if you'd like

  3. patrickvonplaten
    added
    good first issueGood for newcomers
    on Aug 17, 2022

  4. patrickvonplaten commented on Aug 19, 2022

    @patrickvonplaten
    patrickvonplaten
    on Aug 19, 2022
    ContributorAuthor

    That would be great @antival3nt 😍

  5. JC-swEng
    added a commit that references this issue on Aug 31, 2022

    Changed variable name from "h" to "hidden_states"

    bbeddce
  6. patil-suraj
    added a commit that references this issue on Sep 1, 2022

    Changed variable name from "h" to "hidden_states" (#285)

    1f196a0

  7. rashmimarganiatgithub commented on Sep 1, 2022

    @rashmimarganiatgithub
    rashmimarganiatgithub
    on Sep 1, 2022
    Contributor

    @patrickvonplaten , please assign to me.

  8. patrickvonplaten commented on Sep 2, 2022

    @patrickvonplaten
    patrickvonplaten
    on Sep 2, 2022
    ContributorAuthor

    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

  9. natolambert
    added a commit that references this issue on Sep 7, 2022

    Changed variable name from "h" to "hidden_states" (#285)

    dd3889c

  10. daspartho commented on Sep 8, 2022

    @daspartho
    daspartho
    on Sep 8, 2022
    Contributor

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

  11. patil-suraj commented on Sep 9, 2022

    @patil-suraj
    patil-suraj
    on Sep 9, 2022
    Contributor

    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.

    diffusers/src/diffusers/models/attention.py

    Line 140 in b2b3b1a

    def forward(self, x, context=None):

    diffusers/src/diffusers/models/attention.py

    Line 195 in b2b3b1a

    def forward(self, x, context=None):

    diffusers/src/diffusers/models/attention.py

    Line 313 in b2b3b1a

    def forward(self, x):

    diffusers/src/diffusers/models/attention.py

    Line 331 in b2b3b1a

    def forward(self, x):

  12. daspartho commented on Sep 9, 2022

    @daspartho
    daspartho
    on Sep 9, 2022
    Contributor

    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 :)

  13. rashmimarganiatgithub commented on Sep 9, 2022

    @rashmimarganiatgithub
    rashmimarganiatgithub
    on Sep 9, 2022
    Contributor

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

  14. daspartho commented on Sep 9, 2022

    @daspartho
    daspartho
    on Sep 9, 2022
    Contributor

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

  15. daspartho commented on Sep 9, 2022

    @daspartho
    daspartho
    on Sep 9, 2022
    Contributor

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

  16. daspartho commented on Sep 11, 2022

    @daspartho
    daspartho
    on Sep 11, 2022
    Contributor

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

  17. i-am-epic commented on Sep 27, 2022

    @i-am-epic
    i-am-epic
    on Sep 27, 2022
    Contributor

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

  18. patil-suraj commented on Sep 29, 2022

    @patil-suraj
    patil-suraj
    on Sep 29, 2022
    Contributor

    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

  19. i-am-epic commented on Sep 29, 2022

    @i-am-epic
    i-am-epic
    on Sep 29, 2022
    Contributor

    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).

  20. daspartho commented on Sep 29, 2022

    @daspartho
    daspartho
    on Sep 29, 2022
    Contributor

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

  21. i-am-epic commented on Sep 29, 2022

    @i-am-epic
    i-am-epic
    on Sep 29, 2022
    Contributor

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

  22. daspartho commented on Sep 29, 2022

    @daspartho
    daspartho
    on Sep 29, 2022
    Contributor

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

    diffusers/src/diffusers/models/resnet.py

    Line 329 in f10576a

    def forward(self, x, temb):

  23. i-am-epic commented on Sep 29, 2022

    @i-am-epic
    i-am-epic
    on Sep 29, 2022
    Contributor

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

    diffusers/src/diffusers/models/resnet.py

    Line 329 in f10576a

    def forward(self, x, temb):

    @daspartho can u help me what's wrong in my pull request #677
    thanks :)

  24. i-am-epic commented on Sep 30, 2022

    @i-am-epic
    i-am-epic
    on Sep 30, 2022
    Contributor

    Opened new PR #677
    Please do verify, thanks.

  25. dgarnitz commented on May 6, 2023

    @dgarnitz
    dgarnitz
    on May 6, 2023

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

  26. SauravMaheshkar commented on Jun 25, 2023

    @SauravMaheshkar
    SauravMaheshkar
    on Jun 25, 2023
    Contributor

    I raised #3868, fixing some of the last few remaining bits. Any ideas for appropriate names for the Temporal blocks viz. the variable name t ?

  27. patrickvonplaten
    closed this as completedin #3868on Jun 28, 2023
  28. yoonseokjin
    added a commit that references this issue on Dec 25, 2023

    Changed variable name from "h" to "hidden_states" (huggingface#285)

    4cdb17b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestgood first issueGood for newcomers

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions