Fit the contour generate_airfoils is handed, not a second wrap of it - #294
Conversation
Both adapters shrink-wrap a slice with the wrap_method their settings name and pass the result in as x_fit/y_fit, and generate_airfoils then called the convenience method whose job is to wrap a raw slice — a second wrap, with a default ShrinkWrap() rather than the configured one. On the SK100 that degenerates the least-squares Kulfan fit for nine of 45 sections and writes their contours at 1e2 to 1e4 instead of 0..1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017eV7cQVw5begFgMvCkRsW3
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
This is the case that made me want figures in PR cards, so let's do it here first. The argument is entirely visual and the diff is three lines — a reader can't check
Render the before from the merge base in a separate worktree, same inputs and same axes limits as the after. Existing exports in the Makie ext ( Hosting is 1-Bart-1/Agents#226 and may not be ready. If it isn't, say so here and hold — don't commit PNGs to this branch. Keep the existing text; the figures are evidence for it, not a replacement. |
|
The fix is right and the test pins it at δ=0. What it doesn't cover is the δ grid.
Worth adding, here or as a follow-up: one The unregularised fit underneath is #295. |
|
@1-Bort-1 Add a delta range test to this PR |
|
Picked up as |
|
Fix the error, try again |
|
Which example can I run to see an alpha, delta vs Cl plot? |
1-Bort-1
added
agent:queued
|
No example does. These lines from the repo root give you the figure below, in 5 s once the session is warm: using MakieControlPlots, GLMakie, VortexStepMethod
using VortexStepMethod.ObjAdapter
using VortexStepMethod.AirfoilAero: NeuralFoilSolver
yaml = obj_to_yaml(joinpath("data", "ram_air_kite", "ram_air_kite_body.obj"), mktempdir();
n_sections=4, Re=1e6, alpha_range=-10:2:20, delta_range=-10:5:10,
aero_solver=NeuralFoilSolver(), wingtip_distance=0.05, verbose=false)
body_aero = BodyAerodynamics([Wing(yaml; n_panels=16)])
plot_polar_data(body_aero;
alphas=collect(deg2rad.(-10:0.5:20)), delta_tes=collect(deg2rad.(-10:0.5:10)))
Two things that are easy to trip over. The extension trigger is It needs Separately, on this branch: the box could not install the worktree because |
1-Bort-1
added
agent:ci
1-Bort-1
left a comment
1-Bort-1
left a comment
There was a problem hiding this comment.
Independent review (advisory)
Verdict: APPROVE WITH COMMENTS · 0 inline, 0 off the diff
Good
- The fix removes the double-shrink-wrap path by passing already-fitted Kulfan parameters to
generate_airfoil_aero, matching the docstring and caller behavior. - Regression test uses the existing
test_airfoil.datfixture and a tolerance tied to resampling error, failing with the second wrap and passing without it. - CHANGELOG entry records the user-visible geometry change under Fixed.
- Diff stays tightly scoped to the one behavioral fix, its test, and the changelog entry.
Not good
- Test comment at lines 286-287 spans two lines; the rubric asks for one-line inline comments, with rationale kept in the PR description.
opencode, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
|
Local full suite: FAIL (18 min, Julia 1.12.7, one cell of the matrix) |
1-Bort-1
added
agent:queued
1-Bort-1
added
agent:ci
2 participants
TL;DR
generate_airfoilsshrink-wrapped a contour both of its callers had already wrapped, and that second wrap tips the least-squares Kulfan fit into rank deficiency for about a fifth of the SK100's sections, writing their airfoil.datfiles at 1e4 scale. It now fits the contour it is given — which is what its own docstring says it receives — so thewrap_methodthe caller configured is the only one that runs. Closes #293.What was wrong
Both front ends wrap before they call.
obj_to_yamldoes it atobj_to_yaml.jl:221and the Surfplan adapter atSurfplanAdapter.jl:71, each with thewrap_methodits settings name, and both hand the result in asx_fit/y_fit.generate_airfoilsthen calledgenerate_airfoil_aero(solver, x::Vector, y::Vector), whose whole job is toshrink_wrapa raw slice before fitting — and with a bareShrinkWrap(), so the configured wrap was quietly replaced by the default one as well.Wrapping a wrapped contour is not a small perturbation of the fit. The second wrap rounds an already-rounded convex corner into an arc of near-coincident vertices;
smoothed_curvaturedivides the turn across a span of order 1e-16, soresample_arc's curvature-weighted measure blows up and crowds most of the upper stations into one narrow band of x. The upper Bernstein block of the fit matrix then goes numerically singular — on SK100 section 16,cond(A)moves from 1.07e3 to 2.52e11 — andA \ y_normatkulfan.jl:147returns weights of order 1e5. The residual is normal, because those weights cancel on the stations they were fitted to;kulfan_to_coordinatesre-evaluates them on its own cosine grid, where they do not, and y reaches -13013.Nine of the 45 sections come out that way, and the contours written from them —
airfoils/{6,8,9,10,12,16,27,29,43}.dat— carry coordinates from 1e2 to 1e4 where a clean section is 0..1. Nothing in the pipeline notices: the polars still solve, the node counts are still right, the run reports every airfoil as converged, and the geometry is only refused much later by a consumer that maps the mesh onto a structure. InSymbolicAWEModelsthat readsAeroPressure wing 1: max surface-node→point distance is 35776.8× the local chord, exceeding frame_tol_frac=2.0, which is where this was found: it is what stopped 1-Bart-1/BeyondTheSim.jl#17 from building a model at all.How the test pins it
The written contour has to be the fit of the contour that was handed in. Wrapping again inflates the section by the default clearance, 0.006, so the test measures the extremes of the written
.datagainstkulfan_to_coordinates(fit_kulfan_parameters(x_fit, y_fit, LeastSquaresFit()))and allows 1e-4. With the second wrap that gap is 6.27e-3; without it, 6.31e-6 — the resampling error, three orders of magnitude clear of the tolerance. It uses the suite's owntest_airfoil.dat, so it needs no fixture, and it fails on any airfoil rather than only on the sections whose fit happens to blow up.Where I would push back
This removes the trigger, not the fragility. The fit at
kulfan.jl:147is an unregularisedA \ ywith no rank check, no coefficient bound and no check on whatkulfan_to_coordinatesthen produces — whileKulfanBasisindeform.jl:44already ridge-regularises the same kind of solve. Any other input that crowds the resample will do the same thing just as silently, anddeform_sectionstill wraps once more per deflection on a defaultShrinkWrap()rather than the caller's. Raised separately rather than widened into this diff.One thing worth knowing when judging generated tables: a blown-up section is not always obvious from its bounding box.
10_d10.datlooks normalised because it was rescaled by a large chord — its lower surface is a constantmin_clearance/chord. Thickness is the honest check, notx ∈ [0, 1].Verification
airfoils/10.datstarting144.26245312 47009.20858195, and the consumer'smax surface-node→point distance is 35776.8× the local chordtest/airfoil_aero/test_airfoil_aero.jlred before the fix (1 passed, 1 failed), green after (all testsets pass, exit 0)test/obj_adapter/test_obj_adapter.jlnot run locally: this worktree has no juliaserver session of its own — its directory name collides with another task's upstream worktree — so the tests were run from the dependent's environment, which does not carry the suite'sYAMLandAqua. GitHub CI is the authority for the rest of the suite.Scope
+19 / -1 across 3 files: the one call in
src/airfoil_aero/geometry_gen.jl, its regression test, and the changelog entry. No stack — no open pull request here touches either file.From 1-Bart-1/BeyondTheSim.jl#17 · task
BeyondTheSim.jl-17