Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup weight docs #7074

Merged
merged 9 commits into from
Jan 11, 2023
Merged

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Jan 10, 2023

This PR addresses my own comments from #6936 (comment)

Diff looks big but it's only a few lines in conf.py - the rest is just pure s/x/y

Before:

image

After:

image

Also renames the _weight_size key into _file_size to avoid confusion with the actual size occupied in RAM by the weights, which is also something we want to document: #5982. For the same reason I removed the file size info from the main tables, because it may be more relevant to document the weight size there instead of the file size (and there is only so much column space available).

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Just for my understanding: we are just changing the "keys" in the table to avoid showing the underscore which might be confusing and leaks implementation details, right?

Plus, there is a doc failure: https://app.circleci.com/pipelines/github/pytorch/vision/22527/workflows/6df95767-c7cb-4078-9c9b-4235343bdb2d/jobs/1804785?invite=true#step-108-367

@NicolasHug
Copy link
Member Author

we are just changing the "keys" in the table to avoid showing the underscore

Mostly yes. I aslo renamed the (sort-of private) key from _weight_size to _file_size to avoid confusion, and removed the "Size" column from the big tables; this is because what we originally wanted to document was the weights size as in RAM, not the file size (#5982). But there's only so much info we can show in the table so I prefer waiting until we have the RAM usage info. The file size is still available in the individual models doc.

@NicolasHug
Copy link
Member Author

Copy link
Collaborator

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Nicolas!

@NicolasHug NicolasHug merged commit ed2a0ad into pytorch:main Jan 11, 2023
facebook-github-bot pushed a commit that referenced this pull request Jan 13, 2023
Summary:
* _weight_size -> _file_size

* Better formatting of individual weights tables

* Remove file size from main tables to avoid confusion with weight size (as in RAM)

* Remove unnecessary (file size) suffix

* Fix CI error?

* Formatting

Reviewed By: YosuaMichael

Differential Revision: D42500905

fbshipit-source-id: 76000dea83979d89dc53b068b53bf7326b2afbc9

Co-authored-by: Philip Meier <github.pmeier@posteo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants