-
Notifications
You must be signed in to change notification settings - Fork 40
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
Docstrings #362
Docstrings #362
Conversation
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 great, thanks!
Some bike shedding below:
@maleadt I've addressed the comments. Also, do you have any thoughts on whether the |
Maybe a link to the Apple documentation, e.g., https://developer.apple.com/documentation/metalperformanceshaders? I considered suggesting an explanation on the different levels of wrappers (integrations, high-level wrappers, low-level wrappers), but that distinction isn't as clear in Metal.jl as it is in CUDA.jl where the low-level wrappers are auto-generated. Also, MTL doesn't really integrate with many interfaces. So it's probably not that valuable. |
I pushed some improvements based on your suggestions to the |
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.
LGTM; couple of minor nits, let's merge this after.
Co-authored-by: Tim Besard <[email protected]>
Co-authored-by: Tim Besard <[email protected]>
Co-authored-by: Tim Besard <[email protected]>
Done! I'll merge once tests pass or failures unrelated. |
Oh sorry, didn't see your message. Well, we don't have doctests anyway, so this shouldn't matter hopefully 😅 |
Add docstrings for all missing docstrings from the
Metal
module except forMtlDeviceVector
andMtlDeviceMatrix
as I am unfamiliar with them. Also adds docstrings for the storage mode structs fromMTL
as those will potentially be used.I added very basic docstrings for the
MTL
andMPS
modules, but I feel like not having them gives a more useful output (exported names and such).See #358