-
Notifications
You must be signed in to change notification settings - Fork 178
Refine eight notebooks for DLI readiness: restructured sections and added more context #120
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
… 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using \text for code like ndarray, why not just use code font, e.g. backticks?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you ask @shwina to review this? He made this notebook.
| @@ -1,1492 +1,1528 @@ | |||
| { | |||
| "cells": [ | |||
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.
Can you ask @shwina to review this? He made this notebook.
brycelelbach
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.
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?
shwina
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.
Nice work - I reviewed the notebooks on cuDF and cuda.cccl.
At a high level regarding cuDF, we may want to check with RAPIDS engineering/product folks whether we want tutorials to focus on the base cuDF API or the simpler cudf.pandas - or both.
| "source": [ | ||
| "## 1. Introduction\n", | ||
| "\n", | ||
| "In this notebook, we will build a foundation in data manipulation using **Pandas**, the industry standard for Python data analysis. Then, we will transition to **cuDF**, which allows us to run standard Pandas-like code on the GPU.\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.
I would keep the tone here a bit more neutral. Instead of industry standard, let's just call it a popular tool.
| "\n", | ||
| "In Pandas, `.apply()` works because the CPU can execute your Python function one element at a time. On the GPU, this model does not work: a GPU cannot interpret Python bytecode. To make custom functions run on the GPU, cuDF uses Numba to compile your Python function into GPU machine code (PTX). That compilation step imposes strict rules:\n", | ||
| "\n", | ||
| "- The function must be Numba-compilable (pure math only; no Python objects).\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.
This is somewhat inaccurate. cuDF supports more than pure math operations in UDFs. For example, operations on strings are supported.
Perhaps we can point to the docs above regarding the features/limitations of apply() in cuDF.
| "- faster\n", | ||
| "- simpler\n", | ||
| "- more readable\n", | ||
| "- the intended way to use GPUs\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.
| "- the intended way to use GPUs\n" |
.apply() isn't inherently a bad way to use GPUs
| "id": "bf385d3d", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "## 6.3 Transform with Iterators for Memory Efficiency\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.
We introduce the concept of iterators later in this notebook, so we may want to move this example to that section or after it.
|
+1 to all the comments that @shwina left regarding cuDF. The content is a bit out of date too, I have worked on more up to date content regarding cudf and cuml too, not sure if you'll want to include cuml in there too. Here is the altest iteration https://github.com/rapidsai-community/tutorial Now, I'm not representing cudf product, so I'm not sure what do they would like to focus on, getting @btepera's opinion here would be valuable. |
|
It'd be good to include cudf.pandas in this tutorial, particularly since the notebook is already showing how to perform all of these operations in pandas as a starting point. As cudf.pandas has matured in terms of feature completeness, we have leaned more heavily on it (rather than cudf classic) as an entry point for users. If the target audience for this notebook is "users new to GPU-accelerated data science" then I think cudf.pandas makes a lot of sense. |
|
@nv-kriehl based on @btepera comment above. I can happily help modifying the notebook to be more focused on cudf.pandas. If you think this notebook plus script (to learn about profiling and how to use https://github.com/rapidsai-community/tutorial/blob/main/2.cudf_pandas.ipynb |
No description provided.