Repository navigation
Default correct_aoa to true, with the aero-centre flow leaving out only the section's own bound vortex - #400
Draft
1-Bort-1 wants to merge 10 commits into
Conversation
…vortex at the aero centre Bart decided on #318 to default correct_aoa to true. The aero-centre matrix used for the force directions now keeps every other panel's bound filament (Gaunaa, Li & Pirrung 2026, Eq. 2) instead of zeroing all of them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… bound filaments Also list correct_aoa in the Solver docstring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ite on its own axis The correct_aoa default now calls calculate_AIC_matrices! from calc_forces! on every solve, so its temporaries come from the body's buffers. With a near-zero core, a point on a filament's line fell through the colinearity test on rounding noise and came back NaN. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the axis With the aero-centre matrix keeping neighbouring bound filaments, an aero centre on a refined straight segment lies on its neighbours' axes. The core branch normalised the radial vector there and the old guard returned a hard zero, so ForwardDiff lost the slope FiniteDiff sees. The closed form is the same velocity (2e-11 relative) and linear through the axis. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… bound cl_dist is read at alpha_uncorrected, which equals alpha_dist only without correct_aoa. trim_angle's default alpha_tol bounds |CMy| by 3.3e-5 on the trimmable wing, so the 1e-5 check passed by where the bisection landed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drop comments that restate code, name the kernels' work-vector slots correctly, share the uniform-inflow AIC call across the matrix tests and hoist the ForwardDiff probe out of its testset. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1-Bort-1
commented
Sep 30, 2026
1-Bort-1
left a comment
Member
Author
There was a problem hiding this comment.
Independent review (advisory)
Verdict: APPROVE WITH COMMENTS · 2 inline, 0 off the diff
Good
- Matches the plan: the default flips in both
SolverSettingsandSolver, and the Eq. 2 fix is the one-linemodel == LLT && icp == jringchange at body_aerodynamics.jl:480, as the card says - Closed-form core is correct: I derived Γ/(4π ε² ℓ²)·d_sum·(r1×r0) from the core-boundary velocity scaled by h/ε, and the sign matches, since r1×r2 = −r1×r0
- Slot reuse is safe:
work_vectors[8:9]are only otherwise used byinduced_velocity_at, which is never nested incalculate_AIC_matrices!, and the filament kernels only touch slots 1–5 body_aero.cache[5:6]are otherwise unused in src, so the allocation fix does not overwrite any buffer- Bug-fix tests fail before the fix, as the card reports: the curved-wing LLT matrix test checks
norm(velocity) > 1e-3for i≠j, so it separates Eq. 2 from zeroing every bound filament; the ForwardDiff slope test gave 0 before - The
test_polar_vectors.jlandtest_stability.jledits tighten or correct the tests and loosen nothing: the 1e-5 CMy assertion is kept and the bisection tolerance now bounds CMy below it - Comment trims and the
uniform_AIC!helper are behaviour-preserving cleanup around the change, and the card names them
Not good
src/settings.jl:202— Withtrueas the default, every VSMsolve!(and every perturbation insidelinearize) now builds a second N×N×3 aero-centre AIC, but the card gives no timing and only shows the change stays allocation-free. Simulators callingsolve!every step will pay a cost nobody measured.src/filament.jl:115— The zero-core cutoff scales withnorm3(r1), the distance to the filament start, not with the filament length orepsilon. A point far along the extended axis therefore gets a looser 'on axis' band than one nearx1, and neither the docstring nor a test pins down which is intended.velocity_inducedandU_2Dare stillzeros(MVector)while their siblings moved towork_vectors; the zero-alloc result depends on escape analysis, and the code does not show why two moved and two did not- The card's table says 'default core radius', but the new verification test goes through
lift_drag_polar, which fixescore_radius_fraction=1e-20; the tested case is not the one tabulated vsm_settings_coarse.yamlchanges its live value fromfalsetotrue; the card calls this collapsing a duplicate, not a behaviour flip for users of that file- Now that
falseis no longer the default, no test pins thecorrect_aoa=falsepath (LL-3/4); anyone who sets it back to get the old drag has no regression guard - The 'LLT matrix leaves out only…' test rebuilds the AIC from the filament kernels, which is close to the parity-test pattern §2 warns about; a hand-computed expectation for one off-diagonal entry would be independent of the kernels
- The swept-wing no-core test only asserts
isfinite; it would also pass if the fallback zeroed entries it should not
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
…e zero-core fallback Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
Local full suite: PASS (10 min, Julia 1.13.0, one cell of the matrix) |
Takes #393's inflow_angle in update_effective_angle_of_attack! and keeps this branch's own-bound-only aero-centre matrix; BodyAerodynamics.cache holds the three buffers still read (induced velocity, panel areas, AIC inflow). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Plotting changes only; no overlap with this branch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6 tasks done
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
The solver now turns each section force by the flow at its 1/4-chord aerodynamic centre while still reading the polars at the 3/4-chord control point (Gaunaa, Li & Pirrung 2026, TAT2 + TAT3), and that flow now leaves out only the panel's own bound filament (their Eq. 2), not every panel's. With the old default the elliptic AR 4 wing reached an Oswald efficiency of 0.73 against the theoretical 1. It now reaches 0.998.
What was wrong
With
correct_aoa=false, one velocity both read the polars and turned the force, which the paper calls LL-3/4. That keeps lift but loses induced drag.correct_aoa=truealready did TAT2 + TAT3, but its aero-centre matrix zeroed every panel's bound filament (evaluation_point_on_boundwas a global flag), which is a stronger removal than the "undisturbed" flow of TAT3. On a straight wing that makes no difference, because the other bound filaments are colinear with the evaluation point and induce nothing there. On a swept or curved wing it does.Measured on this branch: VSM,
INVISCID(Cl = 2πα), α = 10°, default core radius, e = CL² / (π · AR_projected · CD):false)true, every bound filament zeroedtrue, Eq. 2 (this PR)The regression test runs the AR 4 case through the Python verification settings,
core_radius_fraction = 1e-20: there it gives e = 0.729 withfalseand 0.999 withtrue(CL 0.6498 → 0.6505), and it asserts e within 0.01 of 1 fortrueand below 0.8 forfalse.The middle column shows why the Eq. 2 part matters: on the swept, anhedral wing, zeroing every bound filament halves the induced drag (e = 1.95). CL moves by less than 0.2 % throughout. e > 1 on the non-planar wing is expected, since e is taken on projected span. I have no VLM reference for that case, so 1.110 is not checked against an independent number.
On the V3 kite
CL and the stall angle (14°) do not move, because the polars are still read at 3/4 chord. CD drops most around the L/D peak: at α = 10° it goes from 0.0998 to 0.0742, and max L/D rises from 9.28 to 12.13. The new curve sits on the RANS drag, and the old one sat closer to the wind tunnel. Where I would push back on my own result: the wind-tunnel number is the one a flight simulator is tuned against. Anyone who calibrated to the old drag will see about 25 % less of it, and
correct_aoa: falsegets the old curve back. The "before" is the same branch withcorrect_aoa=false. With that setting the aero-centre matrix is never built, so it runs exactly the pre-PR code path.Eq. 2 also moves
LLTon curved wings, which builds its whole solve on the aero-centre matrix: V3 kite at α = 10°,LLTCL goes from 1.070 to 0.905, now close to VSM's 0.894.What the change turned up
calc_forces!stopped being allocation-free (336 B/call, caught bycalc_forces! is zero-alloc).calculate_AIC_matrices!built two temporary arrays and two escapingMVectors, which did not matter while it ran once per solve but did oncecalc_forces!called it every time. They now come frombody_aero.cache[2:3]andwork_vectors[6:9], and the stale# 64 bytesnote incalc_forces!is gone.test_forwarddiff.jl's POLAR_MATRICES case went from 3e-8 to 1.5e-4 relative error, and FiniteDiff agreed with itself at steps 1e-8 to 1e-5 (−0.107626 vs AD −0.107892), so AD was the one that was wrong. The core branch is now the same velocity in closed form,−Γ/(4π) · d_sum/(ε² ℓ²) · (r1 × r0), which agrees with the old branch to 2e-11 on 10⁴ random points and stays smooth through the axis.core_radius_fraction = 1e-20(as the Python verification cases use), fell through the colinearity test on rounding noise and came back NaN. A zero-core point within rounding of the axis now gets zero. The semi-infinite trailing kernel has the same shape, but its core is not linear in the distance, so the same fix would change its model. No point the solver builds reaches it, so it is velocity_3D_trailing_vortex_semiinfinite! returns NaN on its own axis at its start point, and its core is not linear in the distance #399 rather than part of this PR.test_polar_vectors.jlcomparedcl_distagainstalpha_dist, but undercorrect_aoathat is the aero-centre angle, and the polars are read atalpha_uncorrected. The test now names the angle the polars were read at. That was a test bug, and nothing in it was loosened: both fields are identical withcorrect_aoa=false.test_stability.jl's trim tests checked |CMy| < 1e-5 at a trim bisected to α within 1e-5 rad. With dCMy/dα = 3.34 that bounds |CMy| by 3.3e-5, so the old pass (4.3e-6) was down to where the bisection landed, and the new default's 1.15e-5 failed. Both trims now bisect toalpha_tol=1e-7. The 1e-5 assertion is unchanged, and it now holds by construction.vsm_settings_coarse.yamlcarriedcorrect_aoatwice, commentedtrueand livefalse. It now has one live line, and that line istrue, so anyone loading that file also gets the new behaviour.calculate_AIC_matrices!,update_effective_angle_of_attack!andcalc_forces!are gone, and the four uniform-inflow AIC calls in the matrix tests share one helper.From the review, not done: the "LLT matrix leaves out only…" test builds its expectation from the filament kernels. It checks which filaments the matrix leaves out, not the kernel values, and those are pinned on their own in
test_bound_filament.jl. A hand-computed off-diagonal entry would mean rewriting the test, so it is not part of this PR.AR 4 at 121 sections does not converge with LOOP under either setting (NaN,
FAILURE), so the AR 4 numbers use 41 sections. That is not this PR's doing. It looks like the same family as #395 and #315.Verification
test/verification/test_verification.jl"elliptic AR 4 wing reaches Oswald efficiency 1": red with the old default (22/23), green (23/23); it now also pinscorrect_aoa=false(LL-3/4) below e = 0.8, green 24/24test/body_aerodynamics/test_body_aerodynamics.jl"LLT matrix leaves out only each panel's own bound filament": red on the old flag (9/15), green 15/15. "LLT matrix is finite on a swept wing without vortex core": red (NaN), green, and it now also matches the same matrix built with a 1e-10 coretest/filament/test_bound_filament.jl"ForwardDiff sees the core's slope on the extended axis": red (slope 0.0 vs 0.1414), green 227/227testenv): body_aerodynamics ×2, filament ×2, panel, settings, wake, and solver/{solver, flow_curvature, viscous_drag_correction, attached_trailed_force, moment_units, forwarddiff, polar_vectors, backend_comparison, wing_directions, unrefined_dist, stability}, verificationinflow_angle): one conflict inupdate_effective_angle_of_attack!, resolved to Measure alpha in the plane square to the bound vortex (Gaunaa CP1) #393'sinflow_angleon this branch's own-bound-only matrix. Measure alpha in the plane square to the bound vortex (Gaunaa CP1) #393 had also trimmedBodyAerodynamics.cacheto one buffer, under this branch'scache[5:6], so it holds three again. After the merge: body_aerodynamics ×2, filament bound, solver/{solver, polar_vectors, stability, forwarddiff}, verification all green (5146 assertions,calc_forces! is zero-allocand the AR 4 Oswald test included)calc_forces! is zero-allocincluded), test_forwarddiff 10/10solve!runs on the V3 kite (false → true → false), two were discarded bybench-end(conditions moved, up to 22 of 20 cores busy). The one it kept gave 1.94×, but the same setting timed at either 3.3 ms or 6.7 ms between runs, so I'm not quoting a ratio. Allocations are deterministic: 6 allocations / 208 B persolve!with either setting. The added work is one more N×N×3 aero-centre matrix persolve!, whichfalseskips.Scope
+168 / −91 across 12 files. Stacked on #392, which moves
correct_aoahandling into the solver and removescalculate_results. This PR targets its branch and merges after it.src/holds the two defaults and their docstrings, the one-line Eq. 2 flag, the allocation fix, the closed-form core, and the comment trims. The rest is new tests, thetest_polar_vectors.jlandtest_stability.jlfixes, the yaml collapse and the changelog.Opened by
1-Bort-1, an AI agent working for @1-Bart-1.Closes #318 · task
VortexStepMethod.jl-318