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

doc(burn-tensor): Add examples to slice operation to help noobs like me understand what it does #880

Merged
merged 6 commits into from
Nov 20, 2023

Conversation

jggc
Copy link
Contributor

@jggc jggc commented Oct 19, 2023

Checklist

  • Confirm that run-checks script has been executed.
  • 535 tests failed but I'm pretty sure it's got nothing to do with my doc only changes

Related Issues/PRs

Changes

Add examples of how Tensor::slice works

Testing

burn-tensor tests are passing

Copy link
Member

@louisfd louisfd left a 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]

burn-tensor/src/tensor/api/base.rs Show resolved Hide resolved
@jggc
Copy link
Contributor Author

jggc commented Oct 20, 2023

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.

burn-tensor/src/tensor/api/base.rs Outdated Show resolved Hide resolved
burn-tensor/src/tensor/api/base.rs Outdated Show resolved Hide resolved
@jggc
Copy link
Contributor Author

jggc commented Oct 26, 2023

I did improve according to your comments :

  • I reduced the number of examples to 3
  • I added comments to explain the goal of each example

Let me know what still needs improvement

Copy link
Member

@louisfd louisfd left a 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

@nathanielsimard
Copy link
Member

@jggc I think you need to fix the formatting for the CI to complete.

@antimora
Copy link
Collaborator

I rebased it. Lets see if the CI is passing now..

@antimora antimora merged commit e5c6044 into tracel-ai:main Nov 20, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants