-
Notifications
You must be signed in to change notification settings - Fork 290
Propose to improve p01 and p02
#201
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
base: main
Are you sure you want to change the base?
Conversation
| @@ -1,5 +1,3 @@ | |||
| from memory import UnsafePointer | |||
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.
We didn't need to import memory.UnsafePointer at the first puzzle, thus isn't it built-in?
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.
Even if it's built-in, I think explicitly importing it is still helpful.
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.
I agree with Devin Review's comment: "Pedagogical clarity: For beginners, fewer imports = less cognitive overhead."
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.
That's a valid perspective. However, I believe beginners could actually experience confusion about where these constructs originate. Those encountering this for the first time are likely new to Mojo entirely. If something isn't a reserved keyword, making its provenance explicit should aid understanding.
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.
Pull request overview
This PR aims to clean up unused imports in puzzle 2 files and fix a documentation link. The changes remove imports that are not directly used in the respective files and update a reference link to be more specific.
Changes:
- Removed unused imports (
UnsafePointer,block_dim,block_idx) from solution and problem files - Removed starter code hint in the problem file and adjusted the comment
- Fixed documentation link to point specifically to
introduction_layout_tensor.md
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| solutions/p02/p02.mojo | Removes unused imports while keeping thread_idx which is used in the solution |
| problems/p02/p02.mojo | Removes all imports and starter hint, making the problem harder |
| book/src/puzzle_02/puzzle_02.md | Fixes documentation link to be more specific |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
922885b to
fe9a86d
Compare
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fe9a86d to
c3faea7
Compare
ehsanmok
left a 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.
Thanks Tolga! Everything looks good but please keep the UnsafePointer import as it. It's more of making it future-proof as prelue imports have changed a few times.
c3faea7 to
3d2c42a
Compare
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.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
OK, added |
Thanks a lot for this implementation-based learning environment!
Hi @dunnoconnor! FYI: Puzzle 2's video isn't included in the playlist: https://www.youtube.com/playlist?list=PLh0S94-sJw_5yv_s--TW0Asi6liIy0NGz