Skip to content

[maskedtensor] Adagrad sparse semantics [3/4] #2052

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 26 commits into from
Oct 28, 2022
Merged

[maskedtensor] Adagrad sparse semantics [3/4] #2052

merged 26 commits into from
Oct 28, 2022

Conversation

george-qi
Copy link
Contributor

Restructured the PR stack from #2042

The new format will be:

PR #1: Overview Tutorial

PR #2: Sparsity

PR #3: Adagrad sparse semantics, i.e. what does MaskedTensor make easier -- this one

PR #4: Advanced semantics, e.g. NumPy differences and reduction semantics from Old overview

@netlify
Copy link

netlify bot commented Sep 24, 2022 •
edited
Loading

✅ Deploy Preview for pytorch-tutorials-preview ready!

Name Link
🔨 Latest commit f5b4021
🔍 Latest deploy log https://app.netlify.com/sites/pytorch-tutorials-preview/deploys/635c399be9a4b50008dbe61b
😎 Deploy Preview https://deploy-preview-2052--pytorch-tutorials-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@george-qi george-qi mentioned this pull request Sep 24, 2022
Merged
@janeyx99
Copy link
Contributor

Generally, I got the point of the tutorial eventually, so nice! I do believe there are some opportunities to make the tutorial clearer, including:

  • highlighting/emphasizing points across the tutorial (see my other comments)
  • making the intro less technical --> it may be interesting actually to swap the intro and parts of the conclusion. The conclusion does a better job connecting to the new reader by making the general point that MaskedTensor simplifies code by disentangling the concepts of "sparsity semantics" and "sparsity optimization". The intro currently includes some technical details that do not make it easy for a new reader to immediately know what's up (the one-off implementation details, starting off with an Issue). These details become clearer once someone has gone through the code comparisons, so maybe can go at the end.

@svekars svekars added the 1.13 label Oct 3, 2022
Svetlana Karslioglu and others added 2 commits October 10, 2022 10:02
@george-qi george-qi changed the base branch from master to 1.13-RC-TEST October 20, 2022 17:26
@george-qi george-qi force-pushed the maskedtensor_tutorial_3 branch from f5b048f to e7fbf51 Compare October 20, 2022 17:28
@george-qi george-qi force-pushed the maskedtensor_tutorial_3 branch from e7fbf51 to d3b11e7 Compare October 21, 2022 16:32
@george-qi
Copy link
Contributor Author

I like the comment on switching the conclusion to the front a lot -- I've implemented that :)

@george-qi george-qi force-pushed the maskedtensor_tutorial_3 branch 2 times, most recently from f141231 to d276617 Compare October 21, 2022 18:57
janeyx99
janeyx99 approved these changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@janeyx99 janeyx99 left a comment

Choose a reason for hiding this comment

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

Overall good! Just a few more minor clarifications; the general sense is conveyed.

I haven't verified that the links all work (the preview's not done yet), but spot checking a few seem good.

@george-qi george-qi force-pushed the maskedtensor_tutorial_3 branch 2 times, most recently from e579146 to 25e33cf Compare October 21, 2022 20:43
@george-qi george-qi force-pushed the maskedtensor_tutorial_3 branch from 25e33cf to 304f08d Compare October 22, 2022 00:06
@malfet malfet removed the 1.13 label Oct 26, 2022
@george-qi george-qi added the 1.13 label Oct 26, 2022
@svekars svekars changed the base branch from 1.13-RC-TEST to master October 28, 2022 20:11
@svekars svekars merged commit 1036c92 into master Oct 28, 2022
@svekars svekars deleted the maskedtensor_tutorial_3 branch October 28, 2022 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svekars svekars svekars left review comments

@janeyx99 janeyx99 janeyx99 approved these changes

Assignees
No one assigned
Labels
1.13 cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@george-qi @janeyx99 @svekars @malfet @facebook-github-bot