-
Notifications
You must be signed in to change notification settings - Fork 526
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
Improve documentation page for "Tensor" #2951
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2951 +/- ##
==========================================
+ Coverage 82.11% 82.13% +0.01%
==========================================
Files 867 867
Lines 120268 120268
==========================================
+ Hits 98763 98781 +18
+ Misses 21505 21487 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for improving the docs! This might be a common friction point for new users 🙏
I have a couple of comments, lmk what you think.
/// - If current size is `1` and the requested size is `n` > 1, the tensor will be extended by | ||
/// repeating itself in this dimension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is misleading, because expand does not actually repeat the values (which would imply allocating new memory for the repeated output tensor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm... then I probably did not fully understand this method myself. I will remove this comment to avoid leaving a misleading comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expanding a tensor does not allocate new memory. Instead, it modifies the strides of the tensor to virtually expand it along the specified dimensions.
This is done by setting the stride to 0 for the expanded dimensions, meaning every element along that dimension points to the same memory location. This effectively "repeats" values without duplication.
Hopefully this clears it up a bit 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes clear (and clever)!
Regarding your other remarks: More general remarks about the documentation:
That's a good point! We should probably add at least a small section.
I agree, it would be nice to stick to the same pattern / tense for the operations. It's not a big deal, but standardizing the docs would be nice. I don't have any personal preference for the tense, as long as it is clear 😅
Hahah yeah there might be a slight abuse of language in some instances where dimensions/shape/size is used 😬 If you want to improve the docs to disambiguate the usage of these terms, you can do it in another PR 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for improving the docs 🙂
Improve "Tensor" page documentation
Checklist
run-checks all
script has been executed.Changes
Things to discuss
This is my first PR, I am new to Burn crate and struggled to understand some functions for which I needed to check directly the code. The goal of this PR is to avoid this for next newcomers!
Also if this PR shall be split in several smaller PRs, please tell me. I didn't feel like I should have made one PR per function documented...
More general remarks about the documentation:
rank
,dimension
,dimensions
,size
andshape
is not always consistent, quite often I found "tensor of dimensions [x, y, z]", the idea is totally understandable here, but in this case the right word is "tensor of shape [x, y, z]", no? I am asking just to know if I should try to be accurate in the vocab for next PR (maybe even replace existing misuses) or if it's not important and should not be changed?Testing
Visual testing of the generated doc.