Changing core radius fraction to default 0.05 and adjusting for machi…#155
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the default core radius fraction parameter from a very small value (1e-20) to a physically meaningful value (0.05) based on Damiani et al. (2019) research, and improves floating-point comparison logic in one of the vortex filament methods.
Changes:
- Updated default
core_radius_fractionfrom 1e-20 to 0.05 with scientific reference - Replaced exact equality comparison with tolerance-based comparison for machine precision handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/VSM/core/Solver.py | Changed default core_radius_fraction parameter from 1e-20 to 0.05 with citation to Damiani et al. (2019) |
| src/VSM/core/Filament.py | Improved floating-point comparison from exact equality to tolerance-based check in velocity_3D_trailing_vortex_semiinfinite method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return K * r1XVf | ||
| # if point is on the filament | ||
| elif jit_norm(r1XVf) / jit_norm(Vf) == 0: | ||
| elif jit_norm(r1XVf) / jit_norm(Vf) < 1e-12 * epsilon: |
There was a problem hiding this comment.
The change from exact equality comparison (== 0) to a tolerance-based comparison (< 1e-12 * epsilon) is good practice for floating-point arithmetic. However, similar exact equality comparisons exist in other methods of the same file that should also be updated for consistency:
- Line 105 in
velocity_3D_bound_vortexmethod:elif jit_norm(r1Xr0) / jit_norm(r0) == 0: - Line 182 in
velocity_3D_trailing_vortexmethod:elif jit_norm(r1Xr0) / jit_norm(r0) == 0:
These methods have similar logic for checking if a point is on the filament and should use the same tolerance-based comparison approach.
| allowed_error: float = 1e-6, | ||
| relaxation_factor: float = 0.01, | ||
| core_radius_fraction: float = 1e-20, | ||
| core_radius_fraction: float = 0.05, # Following Damiani et al. (2019) https://docs.nrel.gov/docs/fy19osti/72777.pdf |
There was a problem hiding this comment.
Changing the default core_radius_fraction from 1e-20 to 0.05 represents a significant change (approximately 6 orders of magnitude increase). While the reference to Damiani et al. (2019) provides justification, this is a breaking change that will affect all existing code using the default value.
The test file (tests/Solver/test_solver.py line 318) uses test values of [1e-6, 1e-4, 1e-2], which suggests the old default of 1e-20 may have been far outside the practical range. However, users who rely on the default behavior may experience different results after this change. Consider documenting this as a breaking change in the changelog or migration guide.
1 participant
…ne precision