Skip to content

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Oct 28, 2021

This PR adds a minor update to the side TOC by changing the order and the title of the sections, instead of the non-informative module names (see screenshot) (EDIT: order was upated since screenshot).

This PR also adds a major update which consist in putting each function and class into their own page. This adds a few benefits:

  • it make the pages much shorter, and easier to navigate
  • it makes classes and functions more discoverable, as there is much more that fits into one screen
  • it makes the structure of each page easier to parse and understand, which will greatly help better organizing the docs in the future (see note below about models)
  • it will provide much better google analytics feedback

Note: the model page still needs a significant update, more PRs will come later.

Here are the rendered docs: https://945987-73328905-gh.circle-artifacts.com/0/docs/index.html vs the current docs: https://pytorch.org/vision/stable/index.html

Screenshot 2021-10-29 at 23 54 26

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Oct 28, 2021

💊 CI failures summary and remediations

As of commit 3f48e2c (more details on the Dr. CI page):


  • 2/2 failures introduced in this PR

2 failures not recognized by patterns:

Job Step Action
CircleCI unittest_linux_cpu_py3.6 Run tests 🔁 rerun
CircleCI unittest_macos_cpu_py3.6 Run tests 🔁 rerun

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@NicolasHug NicolasHug marked this pull request as draft October 28, 2021 15:58
@NicolasHug NicolasHug changed the title NORMG Put each transform into its own page Big doc revamp - simplify and improve the subpackage pages Oct 29, 2021
@NicolasHug NicolasHug marked this pull request as ready for review October 29, 2021 23:07
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

I understand that more work is needed to iron out some of the issues but I think this is already a major improvement in terms of readability. I would be in favour of merging it and continue changes on a follow up PR.

Copy link
Contributor

@oke-aditya oke-aditya left a comment

Choose a reason for hiding this comment

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

This is so awesome ❤️ @NicolasHug

I have few thoughts, hope you don't mind 😅


torchvision.ops
===============
Operators
Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to suggest this from long time, current operators page look a bit bloated.
We should probably divide in sections.
E.g. operators on boxes, operators on masks, detection operators, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely, one positive thing about the changes in this PR is that it surfaces our current doc structure weaknesses and it makes them more obvious, which will allow us to fix them.

To keep things manageable though, I would leave this out for future improvements, otherwise there will be too many things to review at once

:template: function.rst

.. autofunction:: draw_segmentation_masks
draw_bounding_boxes
Copy link
Contributor

Choose a reason for hiding this comment

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

And specific reason to change the order?
It's kinda nice to push make_grid down as we don't actively maintain it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's kinda nice to push make_grid down as we don't actively maintain it.

That'st the reason :)

:maxdepth: 2
:caption: Package Reference

models
Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow the brain of users is accustomed to see docs in the previous order.
It might be unpleasant to change to order. I love the previous order. And kind off in presentations related to torchvision, E.g. Francisco's at PyTorch day, he presents by the previous order,

Would love the old order :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The old order is alphabetical, and might not reflect the order of what users may be looking for.

I think that users are more interested in models, transforms and ops than say i.o. operations, so I changed the order to reflect the importance of each section. Obviously it's not set in stone and I'm happy to hear other's thoughts, but I don't think we should worry too much about current readers' habits. It's still very obvious which section corresponds to what, so users will get used to this one fast :)

Copy link
Contributor

@oke-aditya oke-aditya Oct 31, 2021

Choose a reason for hiding this comment

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

So let's arrange according to usage order. Simple LFU caching here 😄

Datasets like MNIST, ImageNet are universal, so probably everyone needs it.
Transforms to convert ToTensor etc Are almost in every code.
Models are not used all places.
Ops are then used a bit more scarce.
Utils, are least used I think,

So the order datasets, transforms, models, ops, utils :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the input @oke-aditya , I will update accordingly


torchvision.models
##################
Models and pre-trained weights
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably write in Title Case. As that's how PyTorch docs on left are

image

Copy link
Member Author

@NicolasHug NicolasHug Oct 31, 2021

Choose a reason for hiding this comment

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

I'm not sure they're using Title Case - see e.g. "Features for large-scale deployments" or "Autograd mechanics".

They're uing "Automatic Mixed Prececision" and "Distributed Data Parallel" and "Frequently Asked Questions" because these are commonly known and shortened AMP, DDP, FAQ, etc.

ops
io
models
feature_extraction
Copy link
Contributor

Choose a reason for hiding this comment

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

I think We should club this with models, just after it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should club this with models

I agree, I think eventually we will move it into the "Models and pre-trained weights" section. It is torchvision.models.feature_extraction after all. But since the models section needs a much bigger revamp, I wanted to leave this out for now

Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

This looks great, thanks!

@NicolasHug NicolasHug merged commit 5b61144 into main Nov 1, 2021
facebook-github-bot pushed a commit that referenced this pull request Nov 1, 2021
…4783)

Reviewed By: datumbox

Differential Revision: D32064695

fbshipit-source-id: 9a5dca1cf2ff702e7a631c473c408fe7ca496670
cyyever pushed a commit to cyyever/vision that referenced this pull request Nov 16, 2021
@datumbox datumbox deleted the autosummary_maybe branch January 12, 2022 20:47
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.

5 participants