docs: Complete Phase 7 - documentation and examples#20
Conversation
There was a problem hiding this comment.
Pull request overview
This PR completes Phase 7 of the cfd-python migration plan by adding comprehensive documentation and examples for v0.1.6 features.
Key Changes
- Updated README.md with complete API documentation for backend availability, boundary conditions, derived fields, CPU features detection, and error handling
- Added migration guide from v0.1.0 to v0.1.6
- Marked Phase 7 as complete in MIGRATION_PLAN.md with effort tracking
- Added 8 new example files demonstrating all major v0.1.6 features
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Added comprehensive API documentation for v0.1.6 features including backend availability, boundary conditions, derived fields, CPU features, and error handling with migration guide |
| MIGRATION_PLAN.md | Marked Phase 7 complete with detailed task checklist and effort tracking; updated total effort to 5.5 days |
| examples/backend_detection.py | Demonstrates CPU feature detection, solver/BC backend queries, and performance recommendations |
| examples/boundary_conditions.py | Shows BC types (Neumann, Dirichlet, no-slip, inlet, outlet) with backend selection |
| examples/channel_flow.py | Channel flow simulation with parabolic inlet profile and Poiseuille flow comparison |
| examples/derived_fields.py | Demonstrates computing velocity magnitude, field statistics, and flow statistics |
| examples/error_handling.py | Shows exception classes, error codes, and raise_for_status helper usage |
| examples/lid_driven_cavity_advanced.py | Complete cavity simulation with convergence monitoring and VTK output |
| examples/solver_comparison.py | Backend performance benchmarking across Scalar, SIMD, OpenMP, and CUDA |
| examples/vtk_output.py | VTK scalar/vector field output, time series generation, and CSV export |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| v[idx] = dx / r * 0.5 | ||
|
|
||
| # Compute velocity magnitude using cfd_python | ||
| vel_mag = cfd_python.compute_velocity_magnitude(u, v, nx, ny) |
There was a problem hiding this comment.
Inconsistent API usage: compute_velocity_magnitude is called with 4 arguments (u, v, nx, ny) here, but with only 2 arguments (u, v) in vtk_output.py lines 142 and 189. The API signature needs to be consistent across all examples and documentation.
| vel_mag = cfd_python.compute_velocity_magnitude(u, v, nx, ny) | |
| vel_mag = cfd_python.compute_velocity_magnitude(u, v) |
| # Compute velocity magnitude | ||
| u = [1.0] * 100 | ||
| v = [0.5] * 100 | ||
| vel_mag = cfd_python.compute_velocity_magnitude(u, v, 10, 10) |
There was a problem hiding this comment.
Inconsistent API usage: compute_velocity_magnitude is shown with 4 arguments (u, v, 10, 10) here, but called with only 2 arguments (u, v) in vtk_output.py lines 142 and 189. The API signature needs to be consistent across all examples and documentation.
| vel_mag = cfd_python.compute_velocity_magnitude(u, v, 10, 10) | |
| vel_mag = cfd_python.compute_velocity_magnitude(u, v) |
| # ================================================================= | ||
| print("\n" + "=" * 60) | ||
| print("Available Derived Field Functions:") | ||
| print(" - compute_velocity_magnitude(u, v, nx, ny): sqrt(u^2 + v^2)") |
There was a problem hiding this comment.
Inconsistent API documentation: The function signature shows compute_velocity_magnitude(u, v, nx, ny) here, but the function is called with only 2 arguments in vtk_output.py. This documentation needs to match the actual API.
| print(" - compute_velocity_magnitude(u, v, nx, ny): sqrt(u^2 + v^2)") | |
| print(" - compute_velocity_magnitude(u, v): sqrt(u^2 + v^2)") |
- Update README with comprehensive API documentation for v0.1.6: - Backend availability API (Scalar, SIMD, OpenMP, CUDA) - Boundary conditions with all BC types and functions - Derived fields and statistics - CPU features detection (AVX2, NEON) - Error handling with exception classes - Migration guide from v0.1.0 - Update MIGRATION_PLAN.md to mark Phase 7 complete - Add new examples: - backend_detection.py - CPU features, solver/BC backend queries - boundary_conditions.py - BC types, backends, inlet/outlet - channel_flow.py - Parabolic inlet, Poiseuille flow comparison - derived_fields.py - Statistics, velocity magnitude, VTK output - error_handling.py - Exception classes, raise_for_status - lid_driven_cavity_advanced.py - Complete cavity simulation - solver_comparison.py - Backend performance benchmarking - vtk_output.py - Scalar/vector fields, time series, CSV output
shaia
force-pushed
the
feature/phase7-documentation-examples
branch
from
f554bfa to
bbf7e64
Compare
- Remove unused create_parabolic_profile function - Simplify compute_analytical_solution by removing unused dp_dx and mu parameters - Update docstring to clarify its purpose as a reference implementation for validation
Use max(1, steps // 10) to avoid ZeroDivisionError when steps < 10.
Consolidate imports to use only 'import cfd_python' and access all symbols through the module namespace, avoiding mixed import styles.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # CFDIOError can be caught as IOError | ||
| try: | ||
| raise CFDIOError(-4, "Cannot write to output file") | ||
| except OSError as e: |
There was a problem hiding this comment.
Line 212 has an incomplete/incorrect sentence in the comment. The comment states "CFDIOError can be caught as IOError" but the actual exception handler at line 212 catches it as 'OSError'. In Python 3, 'IOError' is an alias for 'OSError', so this works, but the comment and code should match. Either update the comment to say "OSError" or update the exception handler to use 'IOError' for consistency with the comment.
| except OSError as e: | |
| except IOError as e: |
|
|
||
| **Returns:** Dictionary with grid info including `x_coords`, `y_coords` | ||
|
|
||
| > **Note:** The stretched grid implementation has a known bug - see ROADMAP.md for details. |
There was a problem hiding this comment.
The note references "ROADMAP.md" but this file name should be in the correct case. Based on the repository context, verify if the file is named "ROADMAP.md" or "Roadmap.md" to ensure the link works correctly.
| > **Note:** The stretched grid implementation has a known bug - see ROADMAP.md for details. | |
| > **Note:** The stretched grid implementation has a known bug - see Roadmap.md for details. |
| against the known analytical solution for fully developed channel flow. | ||
|
|
||
| For Poiseuille flow, the velocity profile is parabolic: | ||
| u(y) = u_max * 4 * (y/H) * (1 - y/H) | ||
|
|
||
| Args: | ||
| nx, ny: Grid dimensions | ||
| u_max: Maximum centerline velocity | ||
| H: Channel height | ||
|
|
||
| Returns: | ||
| list: Analytical velocity field for comparison with simulation | ||
| """ | ||
| u_analytical = [0.0] * (nx * ny) | ||
| dy = H / (ny - 1) | ||
|
|
||
| for j in range(ny): |
There was a problem hiding this comment.
Inconsistency in documentation: The docstring at lines 69-85 states "This is a reference implementation for comparing simulation results against the known analytical solution" and describes the Poiseuille flow formula. However, the implementation doesn't follow the described formula exactly. The docstring states "u(y) = u_max * 4 * (y/H) * (1 - y/H)" but the code implements exactly this at line 92. The issue is that for Poiseuille flow, the maximum velocity should occur at the centerline (y = H/2), and the formula should normalize the maximum velocity correctly. The current implementation is correct, but consider clarifying in the docstring that u_max represents the theoretical maximum at the centerline.
| # Check status | ||
| status = cfd_python.get_last_status() | ||
| if status != cfd_python.CFD_SUCCESS: | ||
| cfd_python.raise_for_status(status, context=f"step {step}") | ||
|
|
||
| except cfd_python.CFDError as e: | ||
| print(f"Simulation error at step {step}: {e}") | ||
| break | ||
|
|
||
| # Get velocity magnitude from result | ||
| vel_mag = result["velocity_magnitude"] | ||
|
|
||
| # Compute statistics | ||
| stats = cfd_python.calculate_field_stats(vel_mag) | ||
| max_vel = stats["max"] | ||
| max_velocities.append(max_vel) |
There was a problem hiding this comment.
The simulation at line 111 runs with 'steps=1' but the result is not actually being used - the returned result contains velocity_magnitude but the code doesn't update 'u', 'v', or 'p' with any new values from the simulation. The code then computes statistics on 'vel_mag' which comes from the library call, but 'u' and 'v' passed to 'compute_flow_statistics' at line 126 are the original setup values, not updated by the simulation. This means the flow statistics don't reflect the actual simulation state. Consider either storing and using the simulation results, or clarifying that this is just demonstrating the API calls.
| u[idx_bottom] = 0.0 | ||
| v[idx_bottom] = 0.0 | ||
|
|
||
| # Top wall (j=ny-1) | ||
| idx_top = (ny - 1) * nx + i | ||
| u[idx_top] = 0.0 | ||
| v[idx_top] = 0.0 | ||
|
|
||
| return u, v, p | ||
|
|
||
|
|
There was a problem hiding this comment.
The comment at line 54 states "Note: bc_apply_noslip applies to all boundaries, so we apply walls specifically" but then the code manually sets velocities to zero for top and bottom walls. However, 'bc_apply_noslip' was already called at line 49, which should have set all boundaries to zero velocity. The manual setting at lines 56-64 is redundant. Either remove the redundant manual setting or clarify the comment to explain why this is being done (perhaps for demonstration purposes).
CHANGELOG updates: - Add Backend Availability API (Phase 5) - Add Derived Fields & Statistics (Phase 3) - Add Error Handling API (Phase 4) - Add CPU Features Detection (Phase 6) - Add CI/Build System (Phase 2.5) - Add Documentation & Examples (Phase 7) - Add new test suites error_handling.py: - Use consistent import style (only 'import cfd_python') - Access all symbols through module namespace
Update comment to reference OSError (the Python 3 name) instead of IOError, and update print statement to match.
channel_flow.py: - Add comment explaining that setup_channel_flow demonstrates BC API but run_simulation_with_params creates its own internal fields error_handling.py: - Add comment explaining __mro__[1:-1] slice behavior - Use _ for unused base variable in loop
Replace __mro__[1:-1] with explicit filtering using a generator expression that excludes the class itself and object. This is more readable and Pythonic.
Add explanation of the factor of 4 in the parabolic velocity profile formula, clarifying that it ensures u_max occurs at the centerline and velocity is zero at the walls.
Remove mixed import styles (both 'import cfd_python' and 'from cfd_python import') and use only 'import cfd_python' with explicit module prefix for all symbols.
The example was computing flow_stats from the initial BC setup fields (u, v, p) instead of the actual simulation results. This was misleading because the "Flow Statistics" and "Simulated max velocity" values didn't reflect the simulation output. Changes: - Use calculate_field_stats(vel_mag) for simulation result statistics - Remove unused flow_stats computation - Update comment to clarify u, v are for BC demonstration only - Replace unused p with _ in tuple unpacking
In Python 3, IOError is an alias for OSError. Updated all references to use OSError for consistency: - examples/error_handling.py: exceptions tuple - tests/test_errors.py: docstring and isinstance check
Make it explicit that the u, v fields created in section 2 are for demonstrating compute_velocity_magnitude() API usage, since run_simulation_with_params() only returns the velocity_magnitude.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - `CFDError`: Base exception class | ||
| - `CFDMemoryError(CFDError, MemoryError)`: Memory allocation failed | ||
| - `CFDInvalidError(CFDError, ValueError)`: Invalid argument | ||
| - `CFDIOError(CFDError, IOError)`: File I/O error |
There was a problem hiding this comment.
The documentation states that CFDIOError inherits from IOError, but the test file at line 95-98 in tests/test_errors.py correctly identifies that it inherits from OSError. In Python 3, IOError is an alias for OSError, so while technically both are correct, OSError is the canonical name and should be used for consistency. The examples/error_handling.py file also correctly uses OSError.
| - `CFDIOError(CFDError, IOError)`: File I/O error | |
| - `CFDIOError(CFDError, OSError)`: File I/O error |
| - `boundary_conditions.py` - BC types, backends, inlet/outlet | ||
| - `backend_detection.py` - CPU features, solver/BC backends | ||
| - `derived_fields.py` - Statistics, velocity magnitude, VTK output | ||
| - `error_handling.py` - Exception classes, raise_for_status |
There was a problem hiding this comment.
The MIGRATION_PLAN.md lists only 4 examples in Phase 7.4 (boundary_conditions.py, backend_detection.py, derived_fields.py, error_handling.py), but the PR description mentions 8 examples total, including channel_flow.py, lid_driven_cavity_advanced.py, solver_comparison.py, and vtk_output.py. The migration plan should be updated to include all examples that were actually added.
| - `error_handling.py` - Exception classes, raise_for_status | |
| - `error_handling.py` - Exception classes, raise_for_status | |
| - `channel_flow.py` - Canonical channel flow simulation with post-processing | |
| - `lid_driven_cavity_advanced.py` - Advanced lid-driven cavity setup with configurable parameters | |
| - `solver_comparison.py` - Compare different solver backends and configurations | |
| - `vtk_output.py` - Standalone VTK output example for visualization pipelines |
Clarify that bc_apply_noslip() is intentionally not used because it would overwrite the inlet/outlet conditions. The manual loop applies no-slip only to top and bottom walls.
The CFDError constructor signature is (message, status_code), not (status_code, message). Fixed the example to match the implementation and test file.
- boundary_conditions.py: Replace Unicode partial derivative symbol with ASCII 'dphi/dn' for Windows console compatibility - output_formats.py: Handle case where CSV file is written to CWD instead of temp directory - vtk_output.py: Replace missing write_csv_field() with inline Python, fix flow_stats key from 'pressure' to 'p'
New example demonstrating CFD result visualization with matplotlib: - Velocity magnitude contour plots - Multi-panel vortex flow analysis (velocity, pressure, vectors, streamlines) - Centerline profile plots - Convergence history monitoring - Grid resolution comparison - 3D surface visualization
- Update all examples to write VTK, CSV, and PNG files to output/ subdirectory - Add examples/output/ to .gitignore - Update examples/README.md with complete list of all 14 examples - Fix convergence plot to show meaningful data (velocity vs steps) Examples updated: - basic_example.py - channel_flow.py - derived_fields.py - lid_driven_cavity_advanced.py - visualization_matplotlib.py - visualization_numpy.py - vtk_output.py
- Add detailed docstring explaining Rankine vortex physics - Update 3D plot title and add explanatory comments - Print note about vortex characteristics when running - Update README to mention Rankine vortex in description The 3D surface plot shows velocity magnitude of a Rankine vortex: - "Volcano" shape with dip at center (zero velocity at axis) - Peak ring at core boundary (r = 0.3) - Decay outside core following 1/r
Enhanced docstrings with educational content explaining: - basic_example.py: CFD fundamentals (grids, time stepping, CFL condition) - channel_flow.py: Poiseuille flow physics, boundary conditions, parabolic profile - derived_fields.py: Velocity magnitude, vorticity, kinetic energy, statistics - vtk_output.py: VTK format types, data types, ParaView visualization - lid_driven_cavity_advanced.py: Cavity physics, Reynolds number effects, convergence - visualization_numpy.py: NumPy array operations, gradients, meshgrid Each example now includes relevant physics background and numerical concepts to help users understand what the code is doing and why.
- Change from ineffective single-step loop to convergence study sampling at key step counts (1, 5, 10, 25, 50, 100, 150, 200) - Document API limitation: run_simulation_with_params() runs fresh simulation each call, doesn't preserve state between calls - Replace unused Re parameter study with meaningful grid resolution study that shows actual variation - Update column headers to reflect actual data (Max Vel, Avg Vel)
- Add backend_detection.py (SIMD/OpenMP/CUDA detection) - Add solver_comparison.py (comparing solver implementations)
Update README with comprehensive API documentation for v0.1.6:
Update MIGRATION_PLAN.md to mark Phase 7 complete
Add new examples: