Skip to content

Conversation

@NicolasHug
Copy link
Contributor

In general we want to avoid having type-annotation-like types within the docstring. Type annotations are meant to be read by a machine, not by humans, and over time they will clutter the docs.

In this instance, int is a more common and familiar term than numbers.Integral, and will help keep the doc more user-friendly. I don't believe there is any loss of accuracy keeping int in the docstring.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 3, 2024
@ahmadsharif1
Copy link
Contributor

Does int convey the message to the user that numpy integers are okay to pass? I believe that was @scotts intent here.

@NicolasHug
Copy link
Contributor Author

I would argue that:

  • We don't need to convey that point
  • numbers.Integral doesn't convey that point either.

I really think numpy integers are an edge case that isn't in the mind of users. I've been using numpy for 10 years and I still don't know what these numpy types are. When int is the documented type, users expect duck typing to prevail, and expect int-like objects to work.

@scotts
Copy link
Contributor

scotts commented Oct 8, 2024

Deferring to @NicolasHug judgement here. Looks good to me!

@NicolasHug NicolasHug merged commit e2ed57c into meta-pytorch:main Oct 8, 2024
@NicolasHug NicolasHug deleted the type-docs branch October 8, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants