-
Notifications
You must be signed in to change notification settings - Fork 192
2-D Navier-Stokes Solver in a periodic box #124
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
|
👋 Thank you for your contribution! This pull request is from a forked repository so GitHub Actions will not be able to run CI. A maintainer will review your changes shortly and manually trigger the CI. @maintainers Please review this PR when you have a chance and follow the instructions in the CONTRIBUTING.md file to trigger the CI. |
|
This pull request contains 8 unsigned commit(s):
All commits must be signed before this PR can be merged. Please sign your commits by following these steps: Option 1: SSH Key Signing (Recommended)
Option 2: GPG Key Signing
For more details, see GitHub's commit signature verification docs. This comment will be updated when you push new commits. |
7521f26 to
8b773ca
Compare
shi-eric
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.
Some initial comments
| "\n", | ||
| "## Overview\n", | ||
| "\n", | ||
| "In this notebook, we will implement a 2-D Navier-Stokes (N-S) solver in a periodic box using the vorticity-streamfunction formulation. The Poisson equation relating streamfunction to vorticity is solved spectrally using tile-based FFT in Warp.\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 intro sentence, while precise, is very technical. We can help novices by at least mentioning that we're building a solver that simulates fluid dynamics.
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 should also emphasize the Python aspect, e.g. something like "We will build our high-performance solver entirely in Python using the Warp framework..."
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.
"In this notebook, we will build a high-performance fluid solver entirely in Python using the Warp framework. Specifically, we will simulate 2-D turbulent flow in a box." -- How does this sound?
| "## Introduction\n", | ||
| "\n", | ||
| "This seemingly simple 2-D N-S solver example combines multiple Warp features that can be leveraged to build industrial-grade solvers:\n", | ||
| " - CUDA kernels for finite difference operators\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.
CUDA kernels should probably be Accelerated kernels for finite-difference operators. When I see "CUDA kernels", I think of all the nastiness associated with CUDA C++, which our users don't need to worry about.
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.
Updated to "Accelerated Warp kernels..."
| "### 1. Vorticity-stream function formulation\n", | ||
| "\n", | ||
| "For 2-D incompressible flow, we define:\n", | ||
| "- **Vorticity**: $\\omega = \\partial v/\\partial x - \\partial u/\\partial y$\n", | ||
| "- **Streamfunction**: $\\psi$ such that $u = \\partial\\psi/\\partial y$, $v = -\\partial\\psi/\\partial x$\n", | ||
| "\n", | ||
| "This formulation automatically satisfies continuity and eliminates pressure from the Navier-Stokes equations.\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 don't explain what the variables u and v are
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 catch, fixed.
| "\n", | ||
| "### 3. Spectral Poisson solver\n", | ||
| "\n", | ||
| "For periodic domains, equation (2) can be directly solved in Fourier space. The wavenumbers are defined as $k_x = \\frac{2\\pi m}{L_x}, \\quad k_y = \\frac{2\\pi n}{L_y}$, where $m$ and $n$ are the Fourier space indices. The vorticity field $\\omega$ is converted to its Fourier representation $\\hat{\\omega}_{m,n}$ using a 2-D FFT. The Poisson equation then becomes algebraic as follows\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.
Maybe you can add some more discussion to help novices. You start off with saying this can be solved in Fourier space, but maybe it's better to acknowledge that it might be tempting to solve the Poisson equation in physical space but we can make use of periodicity to solve the equation in spectral space, which is both efficient and simple to implement.
You can also explain what "Fourier space" is and specifically what the "FFT" means and why it deserves the "Fast" in its name
| "$\\hat{\\psi}_{m,n}$ is then converted back to physical space using inverse FFT.\n", | ||
| "\n", | ||
| "### 4. Time integration \n", | ||
| "For time stepping, we use a strong-stability preserving third-order Runge-Kutta method.\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.
Maybe give a layperson explanation of what SSP and third-order mean
| "\n", | ||
| "### 5. Initial condition: 2-D decaying turbulence\n", | ||
| "\n", | ||
| "In this problem, we solve the governing equations on a square box of dimensions $2\\pi \\times 2\\pi$. The initial condition is generated using the energy spectrum from San & Staples CNF (2012)\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.
Explain why you picked
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.
Done!
| "\n", | ||
| "**NOTE**: Other similar time-stepping schemes can be implemented with minimal changes to the code, but we use the scheme above as a demonstration example.\n", | ||
| "\n", | ||
| "### 5. Initial condition: 2-D decaying turbulence\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 think this is too much. I would create an appendix section and put the details about the initialization there, and then in this section you just say we're gonna use this particular E(k) because of <...> and you can find more discussion behind this choice of I.C. in the appendix.
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 removed the construction of \zeta to the appendix A. I still left the definition of E(k) there.
| "$$\n", | ||
| "\\boxed{\n", | ||
| "\\begin{array}{c}\n", | ||
| "\\textbf{Solver Pipeline} \\\\[10pt]\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 "Solver Pipeline" diagram is already pretty good. I would add a "IFFT" in the first arrow that follows "Initialize
No description provided.