- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.3k
lessons: actually use allocated buffers in Step12 lesson #91
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: master
Are you sure you want to change the base?
Conversation
| the variables pn, un, vn, b... are always re-initialized or reassigned within the main iteration loop or inside functions, and are not shared or referenced elsewhere in a way that would require maintaining their identity replacing direct assignments with slice assignments is not necessary here | 
| I agree that this is not necessary to maintain correctness, but it can improve performance and memory usage in some cases by avoiding unnecessary allocation and reusing allocated slices. | 
| "source": [ | ||
| "def pressure_poisson_periodic(p, dx, dy):\n", | ||
| " pn = numpy.empty_like(p)\n", | ||
| " pn[:] = numpy.empty_like(p)\n", | 
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.
pn doesn’t exist yet, so pn[:] = ... will raise an error
even if it did exist, you’d just be copying junk from a freshly allocated array into pn—no gain here
| "while udiff > .001:\n", | ||
| " un = u.copy()\n", | ||
| " vn = v.copy()\n", | ||
| " un[:] = u.copy()\n", | 
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.
you allocate a temporary copy and then copy it again into un/vn. Double work.
Would suggest this instead (fastest without allocation):
un[...] = u          # or: numpy.copyto(un, u)
vn[...] = v          # or: numpy.copyto(vn, v)
| " vn[:] = v.copy()\n", | ||
| "\n", | ||
| " b = build_up_b(rho, dt, dx, dy, u, v)\n", | ||
| " b[:] = build_up_b(rho, dt, dx, dy, u, v)\n", | 
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.
build_up_b still allocates a new array internally and returns it; then you copy that result into b. That’s an extra full-array copy every iteration
to improve the performance here you would need to refactor build_up_b to write in-place into a provided buffer, something like:
def build_up_b_out(b, rho, dt, dx, dy, u, v):
    b[1:-1, 1:-1] = ...
    b[1:-1, -1]   = ...
    b[1:-1, 0]    = ...
| 
 I think that I know where you are coming from, but the current patch won't result in what you try to achieve. I left a couple comments with some suggestions, hope it helps. | 
No description provided.