Skip to content

Conversation

@shirayu
Copy link
Contributor

@shirayu shirayu commented Sep 12, 2022

@shirayu shirayu marked this pull request as ready for review September 12, 2022 15:16
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Sep 12, 2022

The documentation is not available anymore as the PR was closed or merged.

Copy link
Member

@anton-l anton-l left a comment

Choose a reason for hiding this comment

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

Pretty cool addition IMO, I'm open to try it in manual mode for now, and then implement nightly/weekly typo-fixing PRs if everything goes well :) Thanks @shirayu!
@patrickvonplaten @patil-suraj @pcuenca wdyt?

[default.extend-words]
# Don't correct following words
nin="nin"
nd="np" # nd may be np (numpy)
Copy link
Member

Choose a reason for hiding this comment

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

Look like this is what was meant in those docstrings:

Suggested change
nd="np" # nd may be np (numpy)
nd.array="np.ndarray"

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how this works, from the documentation it looks like it's used to add words to the dictionary (to ignore them), or is it to force corrections? Or both? How was the nd.array typo detected in the first place, was it Typo that pointed it out or did @shirayu saw it themselves and so decided to add it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried typo detection of np.ndarra with this change but it did not work.
My understanding is that typos first tokenize the file and then check using the dictionary.
Therefore, the words that contain . are not recognized well.

_typos.toml Outdated
Comment on lines 4 to 8
[default.extend-identifiers]

[default.extend-words]
# Don't correct following words
nin="nin"
Copy link
Member

Choose a reason for hiding this comment

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

Could you link a line where this appears please?

Copy link
Contributor Author

@shirayu shirayu Sep 13, 2022

Choose a reason for hiding this comment

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

Hereis the result of typos without the line nin="nin".
https://gist.githubusercontent.com/shirayu/43649cc2a3b32c875c40027c02b0c946/raw/44e2be398f73f72cb5971113754c2e4b85e7c177/NIN_at_diffusers

At first I thought nin was an abbreviation for something, but is it a typo for min?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@shirayu shirayu Sep 13, 2022

Choose a reason for hiding this comment

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

Thank you. I fixed use_in_shortcut at b75558b.

-        new_item = new_item.replace("nin_shortcut", "conv_shortcut")
+        new_item = new_item.replace("in_shortcut", "conv_shortcut")

Please check whether this change is OK.

Copy link
Contributor Author

@shirayu shirayu Sep 13, 2022

Choose a reason for hiding this comment

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

Another use of MIN is below.

error: `NIN` should be `INN`, `MIN`, `BIN`, `NINE`
  --> ./scripts/convert_ncsnpp_original_checkpoint_to_diffusers.py:49:76
   |
49 |         new_layer.query.weight.data = old_checkpoint[f"all_modules.{index}.NIN_0.W"].data.T
   |                                                                            ^^^
   |
error: `NIN` should be `INN`, `MIN`, `BIN`, `NINE`
  --> ./scripts/convert_ncsnpp_original_checkpoint_to_diffusers.py:50:74
   |
50 |         new_layer.key.weight.data = old_checkpoint[f"all_modules.{index}.NIN_1.W"].data.T
   |                                                                          ^^^
   |
error: `NIN` should be `INN`, `MIN`, `BIN`, `NINE`
  --> ./scripts/convert_ncsnpp_original_checkpoint_to_diffusers.py:51:76
   |
51 |         new_layer.value.weight.data = old_checkpoint[f"all_modules.{index}.NIN_2.W"].data.T
   |                                                                            ^^^
   |
error: `NIN` should be `INN`, `MIN`, `BIN`, `NINE`
  --> ./scripts/convert_ncsnpp_original_checkpoint_to_diffusers.py:53:74
   |
53 |         new_layer.query.bias.data = old_checkpoint[f"all_modules.{index}.NIN_0.b"].data
   |                                                                          ^^^
   |
error: `NIN` should be `INN`, `MIN`, `BIN`, `NINE`
  --> ./scripts/convert_ncsnpp_original_checkpoint_to_diffusers.py:54:72
   |
54 |         new_layer.key.bias.data = old_checkpoint[f"all_modules.{index}.NIN_1.b"].data
   |                                                                        ^^^
   |
error: `NIN` should be `INN`, `MIN`, `BIN`, `NINE`
  --> ./scripts/convert_ncsnpp_original_checkpoint_to_diffusers.py:55:74
   |
55 |         new_layer.value.bias.data = old_checkpoint[f"all_modules.{index}.NIN_2.b"].data
   |                                                                          ^^^
   |
error: `NIN` should be `INN`, `MIN`, `BIN`, `NINE`
  --> ./scripts/convert_ncsnpp_original_checkpoint_to_diffusers.py:57:80
   |
57 |         new_layer.proj_attn.weight.data = old_checkpoint[f"all_modules.{index}.NIN_3.W"].data.T
   |                                                                                ^^^
   |
error: `NIN` should be `INN`, `MIN`, `BIN`, `NINE`
  --> ./scripts/convert_ncsnpp_original_checkpoint_to_diffusers.py:58:78
   |
58 |         new_layer.proj_attn.bias.data = old_checkpoint[f"all_modules.{index}.NIN_3.b"].data
   |                                                                              ^^^
   |

I think this should not be changed.
I added a memo about this at b7845a3.

Copy link
Member

@pcuenca pcuenca left a comment

Choose a reason for hiding this comment

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

It's surprising the number of typos that were detected!

What does the action do when it runs, does it open a PR like this one, or what would the correction workflow look like? (I'm thinking it could just be a part of the Makefile instead, maybe?)

[default.extend-words]
# Don't correct following words
nin="nin"
nd="np" # nd may be np (numpy)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how this works, from the documentation it looks like it's used to add words to the dictionary (to ignore them), or is it to force corrections? Or both? How was the nd.array typo detected in the first place, was it Typo that pointed it out or did @shirayu saw it themselves and so decided to add it here?

@shirayu shirayu requested a review from anton-l September 13, 2022 01:30
@patrickvonplaten
Copy link
Contributor

Very cool! I'd be happy to add it this one since it should be cheap. But we need indeed to be careful that no "correct" words, definitions are accidently changed

Copy link
Contributor

@patil-suraj patil-suraj left a comment

Choose a reason for hiding this comment

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

Very cool! As @anton-l said we could try this in manual mode to start with :)

@patrickvonplaten
Copy link
Contributor

Good for me if you think it's useful @anton-l :-)

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

Happy to follow @shirayu's and @anton-l's lead here

@anton-l
Copy link
Member

anton-l commented Sep 16, 2022

Ok, let's do a trial run then! :)

@anton-l anton-l merged commit 76d492e into huggingface:main Sep 16, 2022
@shirayu shirayu deleted the feature/fix_typos branch September 16, 2022 15:07
This was referenced Sep 19, 2022
PhaneeshB pushed a commit to nod-ai/diffusers that referenced this pull request Mar 1, 2023
In 749a2c2 iree_device_map and iree_target_map have been made functions
but not all of the uses were updated.
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
* Fix typos

* Add a typo check action

* Fix a bug

* Changed to manual typo check currently

Ref: huggingface#483 (review)

Co-authored-by: Anton Lozhkov <[email protected]>

* Removed a confusing message

* Renamed "nin_shortcut" to "in_shortcut"

* Add memo about NIN

Co-authored-by: Anton Lozhkov <[email protected]>
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.

6 participants