Refine eight notebooks for DLI readiness: restructured sections and added more context #120
Conversation
… added more context
| "source": [ | ||
| "## 4. Aggregations and Axes\n", | ||
| "\n", | ||
| "When performing aggregations (like $\\text{sum}$, $\\text{mean}$, $\\text{max}$), you must specify the **Axis** you want to collapse (or reduce) the array along.\n", |
There was a problem hiding this comment.
Instead of using \text for code like ndarray, why not just use code font, e.g. backticks?
There was a problem hiding this comment.
I think this would especially make sense as it follows what the other notebooks do.
| "if xp == cp:\n", | ||
| " cp.cuda.Stream.null.synchronize()\n", | ||
| "\n", | ||
| "t1 = time.perf_counter()\n", |
There was a problem hiding this comment.
I'm a littel concerned about using %%timeit, %time, and perf_counter. They require us to add stream synchronizes, which means we have to explain them, and it's easy to forget them.
Why not use CuPy's benchmarking facility?
| "%%time\n", | ||
| "# GPU SVD\n", | ||
| "x_gpu = cp.random.random((1000, 1000))\n", | ||
| "u, s, v = cp.linalg.svd(x_gpu)" |
There was a problem hiding this comment.
This is missing a stream synchronize. I think it may be best to use cupyx benchmark instead of %%time.
| " max_steps: int = 400 # Maximum iterations\n", | ||
| " check_frequency: int = 10 # Check for convergence every N steps\n", | ||
| " progress: bool = True # Print progress logs\n", | ||
| " residual_threshold: float = 1e-10 # Stop if error is below this" |
There was a problem hiding this comment.
Please align all the comments so they begin at the same column.
| "# Uncomment to test your implementation:\n", | ||
| "# A_test = generate_device_exercise()\n", | ||
| "# print(f\"Matrix shape: {A_test.shape}\")\n", | ||
| "# print(f\"Matrix is on GPU: {isinstance(A_test, cp.ndarray)}\")" |
There was a problem hiding this comment.
The original notebook was designed to highlight that NumPy and CuPy random number generation give you completely different results. I don't see that preserved in this version. Is there a place where we show that you get different answers for generate_host and generate_device? I think it's an important point to make.
| "provenance": [] | ||
| }, | ||
| "kernelspec": { | ||
| "display_name": "Python 3 (RAPIDS 25.10)", |
There was a problem hiding this comment.
This is the wrong kernelspec for this notebook. It should be "display_name": "Python 3 (ipykernel)". We don't have a RAPIDS 25.10 kernel in the Docker image. Please make sure this is fixed for all of the notebooks.
| @@ -1,2436 +1,667 @@ | |||
| { | |||
| "cells": [ | |||
There was a problem hiding this comment.
Can you ask @shwina to review this? He made this notebook.
| @@ -1,1492 +1,1528 @@ | |||
| { | |||
| "cells": [ | |||
There was a problem hiding this comment.
Can you ask @shwina to review this? He made this notebook.
brycelelbach
left a comment
brycelelbach
left a comment
There was a problem hiding this comment.
These changes look good. I left some comments on feedback where I think changes may be needed, please take a look.
One broader remark: I noticed that you haven't updated the solution notebooks. The solution notebooks are copies of the exercise notebooks that have the exercises filled in. We use them to demonstrate to the class and to test the content in CI. Can you update them as well?
No description provided.