Skip to content

Add Dynamic Shared Memory example. - #8839

Merged
sklam merged 5 commits into
numba:mainfrom
k1m190r:shared_mem_example
Mar 23, 2023
Merged

sklam merged 5 commits into
numba:mainfrom
k1m190r:shared_mem_example

Conversation

@k1m190r

@k1m190r k1m190r commented Mar 22, 2023 •

Copy link
Copy Markdown
Contributor

@gmarkall have provided excellent demonstration of dynamic shared memory as answer to my question on numba discourse. It is essentially copy and paste of @gmarkall 's answer.

This PR partially addresses #5025.

Fixes #5025.

@gmarkall gmarkall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for this! This is something I've been meaning to get round to for literally years but never quite managed it (instead answering the same question multiple times over the years!). I think this is looking generally great - I have made some suggestions on the diff, which are mostly about formatting and grammar / spelling, but a couple of more substantive suggestions too. In addition to the comments on the diff, the lines need wrapping at 80 characters as well - this isn't enforced by a style check in CI, but is the convention for the docs.

I have manually checked that the code snippets execute correctly and demonstrate the correct output. Ideally new code in the docs would be a doctest, but I'd suggest we just fix up the things referred to in the comments here and then could convert to a doctest in a future PR, so that we can get this over the line quickly.

Once you've made adjustments, please ping me and I can do another round of review!

Comment thread docs/source/cuda/memory.rst Outdated
Comment thread docs/source/cuda/memory.rst Outdated
Comment thread docs/source/cuda/memory.rst Outdated
Comment thread docs/source/cuda/memory.rst Outdated
Comment thread docs/source/cuda/memory.rst Outdated
Comment thread docs/source/cuda/memory.rst
Comment thread docs/source/cuda/memory.rst Outdated
Comment thread docs/source/cuda/memory.rst Outdated
Comment thread docs/source/cuda/memory.rst
Comment thread docs/source/cuda/memory.rst
@gmarkall gmarkall added 4 - Waiting on author Waiting for author to respond to review CUDA CUDA related issue/PR doc labels Mar 22, 2023
@gmarkall gmarkall added this to the Numba 0.57 RC milestone Mar 22, 2023
@k1m190r

k1m190r commented Mar 22, 2023

Copy link
Copy Markdown
Contributor Author

@gmarkall all changes as suggested. Thanks for the review!

@gmarkall gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Mar 23, 2023

@gmarkall gmarkall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for the updates - there are a couple of other minor points that I missed last night, if you'd be OK to make those changes.

I didn't know about pycon code blocks, so that was a mistake on my part to suggest changing them to python - feel free to change them back to pycon if you'd prefer.

Comment thread docs/source/cuda/memory.rst Outdated
Comment thread docs/source/cuda/memory.rst Outdated
@gmarkall gmarkall added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Mar 23, 2023
@k1m190r

k1m190r commented Mar 23, 2023

Copy link
Copy Markdown
Contributor Author

Changes committed. I've decided to use pycon to convey meaning of the code-blocks. Though there is no differences in appearance :).

Thanks for the review @gmarkall.

@gmarkall gmarkall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, many thanks for fixing this longstanding issue!

@gmarkall gmarkall added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on author Waiting for author to respond to review labels Mar 23, 2023
@k1m190r

k1m190r commented Mar 23, 2023

Copy link
Copy Markdown
Contributor Author

Yeehaa 1st Numba PR :)

@k1m190r k1m190r closed this Mar 23, 2023
@k1m190r k1m190r reopened this Mar 23, 2023
@k1m190r

k1m190r commented Mar 23, 2023

Copy link
Copy Markdown
Contributor Author

Closed it accidently. Should I keep this open and it'll auto-close when merged?

@gmarkall

Copy link
Copy Markdown
Member

Yeah, it will close when merged - the current state of things is all good.

@gmarkall

Copy link
Copy Markdown
Member

I've linked this to #5025 so it will close too - I think this is sufficient documentation for dynamic shared memory, so we might as well have one less issue on the tracker 🙂

@sklam
sklam merged commit e520da8 into numba:main Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to merge Review and testing done, is ready to merge CUDA CUDA related issue/PR doc

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide documentation, docstrings, tests, examples for dynamic shared memory

3 participants