Give the semi-infinite trailing vortex the bound kernel's linear core, from one shared core definition - #408
Conversation
velocity_3D_trailing_vortex_semiinfinite! now uses the same zero-core guard and closed-form core as velocity_3D_vortex_segment!: zero at its start point rather than NaN, and inside the core the velocity scales linearly with the distance to the axis, so ForwardDiff sees its slope there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1-Bort-1
left a comment
There was a problem hiding this comment.
Independent review (advisory)
Verdict: REQUEST CHANGES · 2 inline, 0 off the diff
Good
- The math is right: at axis_distance = ε, |r1|² = (r1·Vf)²/|Vf|² + ε², so the new inside-core K equals the outside Biot–Savart K there; checked by hand against lines 211 and 213–214.
- The zero-core guard at lines 206–207 is copied exactly from
velocity_3D_vortex_segment!(src/filament.jl:113–115). Atx1, nr1 = ε = 0, so the kernel returns zero instead of NaN. - ForwardDiff works on the axis: in the core branch the non-differentiable
axis_distanceis only compared, never multiplied, so the slope comes from the linear r1×Vf. - Work vectors 4 and 5 are really gone:
rg 'work_vectors\['in src shows the kernel now reads only [1] and [3]. - The docstring now includes the
!, anddocs/src/private_functions.md:81lists the!name, so@docsnow matches it. - The test that pinned NaN at the start point now asserts
== zeros(3). The dead inside-core branch ofanalytical_solutionand itsvaargument are removed. The new testsets name the behaviour they protect. - The changelog entry splits into Changed (model) and Fixed (NaN, zero slope), which matches the card.
Not good
src/filament.jl:213— This inside-core closed form and the guard at 206–207 are a second copy of the per-end core termd/√(d²/|r0|²+ε²)/(ε²|r0|²)and the guard invelocity_3D_vortex_segment!(src/filament.jl:113–134). §2 'one source per equation' calls that the defect. Extract the guard and the per-end core term into one helper that both kernels call; as written, a later fix to one kernel will silently miss the other.test/filament/test_semi_infinite_filament.jl:31—core_radiuscopies the kernel's Lamb–Oseen formula into the test, so the continuity and linearity tests pass whenever the two copies agree, not whenever the kernel is right. Call a shared src function instead, or hard-code the ε = 6.10e-3 m value the card measured.- The card does not say what was searched for before the kernel code was written (§2 'Say in the PR what you searched for'). Had it searched, the duplication in the finding below would have shown up.
sqrt(4 * ALPHA0 * NU * axial / va)now exists in three places: line 200,velocity_3D_trailing_vortex!at src/filament.jl:86, and the test helpercore_radius. It should be one function that the test calls too.- In the ForwardDiff testset,
@test slope != 0adds nothing because the≈finite-difference check below it already covers it; delete it. - The new docstring still does not say what
Vf,vaandwork_vectorsare or which units they use (va[m/s]). The bound trailing kernel's docstring lists its arguments. - The card calls this 'one core model' for all three kernels. The model is shared, but the code is two copies, so the claim is only half true.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| @inbounds for k in 1:3 | ||
| vel[k] = K * cross_tmp[k] | ||
| end | ||
| K = GAMMA / (4π) / (nVf * epsilon)^2 * |
There was a problem hiding this comment.
MAJOR: This inside-core closed form and the guard at 206–207 are a second copy of the per-end core term d/√(d²/|r0|²+ε²)/(ε²|r0|²) and the guard in velocity_3D_vortex_segment! (src/filament.jl:113–134). §2 'one source per equation' calls that the defect. Extract the guard and the per-end core term into one helper that both kernels call; as written, a later fix to one kernel will silently miss the other.
There was a problem hiding this comment.
Fixed in 32243fa: both kernels now build the core from core_end_term and core_coefficient and check the zero-core case with on_axis_without_core (src/filament.jl). Bound and finite-trailing kernels are bit-identical to before, and the semi-infinite one is within 1.4e-15 relative (200k random points).
|
Local full suite: PASS (6 min, Julia 1.13.0, one cell of the matrix) |
…ament kernels Both velocity_3D_vortex_segment! and velocity_3D_trailing_vortex_semiinfinite! now assemble their inside-core velocity from core_end_term and core_coefficient and guard the zero-core case with on_axis_without_core. lamb_oseen_core_radius replaces the two copies of the Lamb–Oseen formula and the test's own copy. The semi-infinite docstring lists its arguments and units. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TL;DR
velocity_3D_trailing_vortex_semiinfinite!now returns zero at its start point instead of NaN. Inside its core the velocity now grows linearly with the distance to the axis instead of staying at the core-boundary value. Its core is built from the same helpers asvelocity_3D_vortex_segment!in #400, so all three filament kernels use one core model and one copy of the code for it.What was wrong
The Lamb–Oseen core radius of a semi-infinite trailing vortex grows from zero at
x1. AtXVP == x1, the "outside the core" test and the< 1e-12 * epsilonguard were both false. The core branch then normalised a zero radial vector and returned[NaN, NaN, NaN], and an existing test pinned that NaN. Inside the core the kernel returned the core-boundary velocity unscaled. A point at a quarter of the core radius got the same speed as one at half of it (ratio1.0), and ForwardDiff saw a slope of0.0on the axis.What changed
src/filament.jl, called by the bound/finite-trailing kernel and by the semi-infinite kernel:lamb_oseen_core_radius,on_axis_without_core(the zero-core guard),core_end_term(one end'sd/√(d²/|r0|²+ε²)) andcore_coefficient. The semi-infinite core iscore_coefficient(Γ, 1 + core_end_term(r1·Vf, …), …): Default correct_aoa to true, with the aero-centre flow leaving out only the section's own bound vortex #400's closed form with the far end's term at its limit.1e-12·|r1|, the velocity is zero. Inside the core it matches the Biot–Savart value atεand scales linearly with the distance to the axis. Its docstring now listsVf,XVP,GAMMA,vaandwork_vectorswith units. The docstring also named the function without its!, so@docsdid not match it. That is fixed.lamb_oseen_core_radiusinstead of keeping its own copy of the formula. The test'sanalytical_solutionlost its inside-core branch, which was the old model and was never called.sqrt(4·ALPHA0·NU·…)was written out in three places and the core term in two. Before extracting the helpers I greppedsrc/andtest/forALPHA0,epsilon^2,1e-12andcore_radius. Nothing else computes them.This is the model change #399 asked about. It moves any velocity evaluated inside a trailing wake core. Nothing the solver builds today is evaluated there. In the previous round the V3 kite polar (verification setup, 36 panels, α = 3/6/9°) gave bit-identical CL and CD under VSM and LLT, old kernel against new, in the same session. This round's refactor leaves
velocity_3D_bound_vortex!andvelocity_3D_trailing_vortex!bit-identical. The semi-infinite kernel moves by at most 1.4e-15 relative, over 200 000 random points from 1e-6 m to 10 m off the axis.Review
I took all seven findings. The shared core helpers and the shared radius function are above. The redundant
@test slope != 0is deleted, because the finite-difference≈right below it already fails for a zero slope. The docstring now lists the arguments. "One core model" is now true of the code as well as the model.Verification
[NaN, NaN, NaN];v(ε/4)/v(ε/2) = 1.0;dv/ddon the axis= 0.0(ε = 6.10e-3 m at 0.5 m, va = 1)test/filament/test_semi_infinite_filament.jl: red before (4 failed / 63), green after (62/62, the one fewer being the deletedslope != 0). New testsets check that the velocity is linear inside the core, continuous at the core boundary, and has a nonzero ForwardDiff slope on the axis. The start-point test now asserts zero.test/filament/test_bound_filament.jl227/227,test/body_aerodynamics/test_body_aerodynamics.jl4925/4925,test/verification/test_verification.jl24/24,test/solver/test_forwarddiff.jl10/10 (juliaserver, on 32243fa)UndefVarError: lamb_oseen_core_radius. It had started at 14:19, while this round's edits were in progress, and loaded the new test file against the oldsrc. It did not test a committed state.@inlineScope
+123 / −61 across 4 files against #400. Of the
src/filament.jlgrowth, 34 lines are the four helpers and their docstrings. The kernels themselves are shorter. The rest is three new testsets and their helper, and 4 lines indocs/src/private_functions.md. Stack: on #400 (the bound-kernel closed form), and this PR targets its branch.Opened by
1-Bort-1, an AI agent working for @1-Bart-1.Closes #399 · task
VortexStepMethod.jl-399