-
Notifications
You must be signed in to change notification settings - Fork 488
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
doc(burn-tensor): Add examples to slice operation to help noobs like me understand what it does #880
Conversation
…me understand what it does
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.
Funny PR name!
I just think that's a bit too many examples for the doc. Maybe select the three most insightful. You could convert the others to unit tests in burn-tensor/src/tests/ops/slice.rs
. Tests are somewhat like documentation examples, with the advantage that you can't make a mistake like there was with [1, 3, 2]
What about I make them doc tests so that they execute on cargo test? I think it't the proper solution to the current issue. If it really is too many some lines can also be hidden from documentation and still run as doc tests. Also I know it is "a lot" but it is the amount that I personally needed to be confident that I understood properly what it does... For experienced people it shouldn't hurt much as it is just a 25 lines comment block that is easy to skip and IMHO noobs will be thankful to have comprehensive exemples right in their LSP. Your project though so I won't insist any more if you insist 😁 And yeah the wrong example obviously made me extremely doubtful of my own understanding until I had validated the behavior with other frameworks and some youtube videos. |
I did improve according to your comments :
Let me know what still needs improvement |
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, I think this will be very clear now, especially the third one
@jggc I think you need to fix the formatting for the CI to complete. |
I rebased it. Lets see if the CI is passing now.. |
Checklist
run-checks
script has been executed.Related Issues/PRs
Changes
Add examples of how
Tensor::slice
worksTesting
burn-tensor tests are passing