Conversation
A self-crossing contour can reach a bare Fortran STOP inside XFoil, which ends the Julia process with exit code 0 and no exception. `analyze_sweep` now checks the contour before `set_coordinates` and throws `ArgumentError` instead, so a section XFoil has no solution for is one failed sweep rather than the end of the session. The same check covers a contour with more nodes than XFoil's panel arrays hold: ABCOPY refused it and left the previously loaded airfoil in place, so every angle came back solved on the wrong shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018rHHzpuPmi8pBjVk3r5QZD
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Merge main into this branch |
|
Fix the failing test and conflicts @1-Bort-1 |
|
Picked up as |
# Conflicts: # CHANGELOG.md
|
The description above is this branch as it stands. Commits
|
|
Merged |
1-Bort-1
left a comment
There was a problem hiding this comment.
Independent review (advisory)
Verdict: APPROVE WITH COMMENTS · 2 inline, 0 off the diff
Good
- Single chokepoint: grep shows no
Xfoil.*call site outsidexfoil_solver.jl, andanalyze_sectiondelegates toanalyze_sweep, so the guard cannot be bypassed. Xfoil.IQX = 286confirmed in the installed package (Xfoil/n7wnc/src/Xfoil.jl:12), so the 281-node bound matches XFoil'sABCOPYrefusal the card describes.- Order is right: validating before
set_coordinatesis the only place it works, since a refused contour leaves the previous airfoil loaded andpaneruns afterset_coordinates. - No duplication: grepped
intersect|cross|orient|signed|detoversrc— the repo had no segment-intersection or side-of-line helper, only plane crossings inobj_slice.jl. - Regression test throws before any Fortran call, so it is fast and cannot itself kill the process; it covers both classes plus a clean control contour.
- Docstrings state only what, no why; CHANGELOG entry sits under
Unreleased/Fixedat the neighbours' length; all four new private names are added toprivate_functions.mdso docs stay complete. - Scope matches the card (5 files, guard + 3 helpers + 1 testset + changelog + docs), tree clean,
.agent/plan.mduntracked.
Not good
src/airfoil_aero/airfoil_solvers/common.jl:148—crossing_panelsassumes a wrap-around closed contour: it never tests the closing segment node n → node 1 and it skips the pair (1, last_panel). That holds fordeform_sectionoutput (shrink_wrapends atpx[1], so first == last), butcompare_live_polarbuilds its section straight fromkulfan_to_coordinates, which leaves first ≠ last for any non-zeroTE_thickness— there panels 1 and n-1 are genuinely non-neighbouring and the trailing-edge segment is untested, so a fold at the TE passes the guard and can still reach theSTOP. For a truly closed contour the skip is dead anyway, since the shared node makes the strict test false.src/airfoil_aero/airfoil_solvers/xfoil_solver.jl:62— Throwing here aborts batches that were built to degrade instead:generate_airfoil_aeroloops deltas and takes the modal node count specifically so "a single degenerate wrap cannot drop every other column", andcompare_live_polar's docstring promises a bad reference "comes backNaNrather than throwing, so a sweep over panels does not stop at the first one that fails". One self-crossing delta or panel now fails the whole grid or panel sweep; the card notes the behaviour change but not that these two callers document the opposite contract.- The three geometry helpers sit in
airfoil_solvers/common.jl(shared-backend file) although their only caller is the XFoil backend. - Testset name says "XFoil refuses" while XFoil is never reached — the refusal is entirely ours.
side_of_line(a, b, p) -> Float64in the docstring, but the function is generic in its argument types.- No end-to-end assertion here that a valid contour still sweeps; that is left to
test/solver/test_backend_comparison.jl. - The card's disclosed behaviour change (four of nine contours that used to return now throw) gets no caller-side handling anywhere.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Solving the cause rather than the effect |
|
Question from byuflowlab/xfoil_light is not a repo this box may push to. Asked on Slack, and answered there. |
|
Opened in the repository this belongs to: |
|
Fixed at the cause upstream in 1-Bort-1/xfoil_light@63a3065, with the pull request against byuflowlab/xfoil_light opened from it. |
|
Local full suite: FAIL (0 min, Julia 1.13.0, one cell of the matrix) |
#320 closed unmerged, its fix having gone to xfoil_light instead. This branch carried it only as a base; the shrink wrap's warning still uses crossing_panels. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TL;DR
analyze_sweep(::XFoilSolver, ...)checks the contour before handing it to XFoil and throwsArgumentError, because a self-crossing contour can reach a bare FortranSTOPin there and end the Julia process with exit code 0 and no exception. The same check catches a contour with more nodes than XFoil's panel arrays hold, which XFoil refused while leaving the previously loaded airfoil in place, so every angle came back solved on the wrong shape.What was wrong
Found from 1-Bart-1/BeyondTheSim.jl#53: an example script ended its Julia process partway through, silently, exit code 0, no stacktrace — in a REPL it takes the session away, in a script it looks like a successful run that printed half its output. Bisecting the call,
Xfoil.set_coordinatesreturns normally andXfoil.solve_alpha(0.0, Re; reinit=true)never does. Exit 0 with nothing on stdout or stderr is a gfortranSTOPwith no message.The contours that do it come out of
deform_section, whose re-wrap of an already-wrapped thin section produces a curve whose upper and lower surfaces cross. That is not an airfoil, and a panel method has no solution for it. Of nine shapes sliced from one mesh section at nine rolling-ball radii, four returned and five took the process with them, while every geometric statistic I measured (shortest panel, neighbouring-panel ratio, maximum turn, crossing count) put survivors and killers in the same range:.dat, one wrap of the same cloudThe rows at 0.05 and 0.10 agree on every column and disagree on life and death, because the
STOPis not geometric. It is the silent one in XFoil'sIBLPAN(xfoil_lightsrc/xpanel.f:1432, its message lines commented out), which fires when the longer boundary-layer side plus the wake exceedsIVX = 229stations. On a self-crossing contour the stagnation point lands beside the trailing edge, so one side takes nearly all N nodes; it cannot fire below 203 nodes. Predicting that from the inviscid solution matched 9 of 9 synthetic runs (a figure-eight and a rear-swapped NACA 0012 die at 220-250 nodes and return NaN at 160-200). The guard enforces the precondition a panel method actually has, a simple closed curve, rather than predicting that overflow.A contour that does not already end on its first node is closed by a panel back to it before the test, so a trailing-edge gap does not hide a fold there. That case is not hypothetical:
compare_live_polarbuilds its section straight fromkulfan_to_coordinates, which atTE_thickness=0.02returns first(1.0, 0.01)and last(1.0, -0.01).I do not claim this makes
analyze_sweepproof against every contour XFoil mightSTOPon: a simple closed contour over 202 nodes whose stagnation point moves far from mid-contour could still overflowIBLPAN. It ends the class a shrink wrap produces, and turns a process death into a failure the caller can catch. The cause is fixed upstream in byuflowlab/xfoil_light, which this branch does not wait on. The upstream branchIBLPANchecks the bound before writing and returns the point unconverged instead of stopping (1-Bort-1/xfoil_light@63a3065). Once it reaches axfoil_light_jllrelease, such a contour givesconverged = falserather than ending the process. The guard here still earns its place then: a self-crossing contour's "solution" is NaN or nonsense (cm = -19on one of them), andArgumentErrornames the crossing panels.The second defect in the same function
While bisecting I found that a contour with more than
Xfoil.IQX - 5= 281 nodes is refused by XFoil'sABCOPY, which prints to stdout and returns, leaving the previously loaded airfoil in the panel arrays.analyze_sweepnever looked, so it solved the sweep on whatever shape was loaded before and returned it as this one. Feeding a NACA 0012 at 299, 339, 399, 499 and 569 nodes gavecl = 0.5604994052186286every time — the 279-node answer, to the last digit. Same function, same idea (say when XFoil did not take the contour), so it rides here.Where I would push back
The real bug is upstream of this:
deform_section's re-wrap should not produce a self-crossing contour in the first place, and until it stops doing so these sections have no XFoil polar at all rather than a poor one. That is the shrink wrap's own concern and would change what every generated polar means, so it is #321 rather than this diff.The contour test is O(n²) in the node count — 239 nodes is ~28k segment pairs, run once per sweep against a viscous march of many angles. I did not benchmark it; it is not on any hot path I can find, and
compare_live_polar's per-panel call is still one XFoil sweep per check.The throw does not take a batch down with it where one is running:
generate_airfoilsalready wrapsgenerate_airfoil_aeroin try/catch and, with the defaultreuse_valid_airfoils=true, warns and reuses the nearest valid airfoil, so one self-crossing delta costs that airfoil rather than every column of the grid.compare_live_polarhas no such catch and a panel whose shape is not a simple closed curve now stops a sweep over panels there — before this branch that same panel ended the process, so nothing graceful is being given up, but that is the caller where somebody will want a catch first.The three predicates
side_of_line,segments_crossandcrossing_panelssit inairfoil_solvers/common.jlalthough only XFoil calls them today. They say nothing about XFoil and belong besideDeformedSectionanddeform_section, where the shared section geometry already is; what is XFoil's own policy,validate_xfoil_contour, is inxfoil_solver.jl. The end-to-end check that a valid contour still sweeps istest/solver/test_backend_comparison.jl, the suite's only XFoil march — the new testset pins the guard itself, that it accepts a closed contour and one with a trailing-edge gap alike.Verification
Xfoil.set_coordinatesok, thensolve_alpha(0.0, Re; reinit=true)never returns — process gone, exit 0, no Julia error and no Fortran messagetest/airfoil_aero/test_airfoil_aero.jlred before, green after (8/8 new assertions, exit 0). Red was run with the source change stashed: the folded contour returned instead of throwing, and the 399-node contour printedMaximum number of panel nodes : 281 / Current airfoil cannot be set.and still returned a solution. The trailing-edge panel was red on the unpatched function too —crossing_panels([0,1,1,2],[0,2,-2,1])gavenothing, and gives(2, 4)nowArgumentErrornaming the crossing panels, all nine in one process where each killer previously needed its own; the valid one still runstest/solver/test_backend_comparison.jlPASS 15/15, the suite's only XFoil march ·test/airfoil_aero/test_live_polar.jlPASS 93/93origin/mainat 9d0f44ed1ffa0c: PASS 6721/6722 (1 pre-existing broken), 8m34s, exit 0, Julia 1.13.0 — the new testset 8/8 andForwardDiff linearize10/10, the latter being the check that was red on Windows before the branch merged main (Flaky POLAR_MATRICES forwarddiff test on Windows / Julia 1.12 (knot-proximity in piecewise-linear polar interpolation) #360, fixed there). The box's earlier red run on this commit ended inKILLED by SIGTERM after 4mwhile compilingtest_plotting.jl, sent from outside the run, and not in a test; theProject.toml … name and a UUIDerror it reported comes fromPkg.testafter that kill.d1ffa0c: all 7 checks green, Julia 1.12 windows includedbin/reuse_lintScope
+101 / -1 across 5 files: the check and its three geometry helpers in
airfoil_aero/, one regression testset, one changelog entry under## Unreleased, four@docsentries. The regression test uses a synthetic folded Kulfan section rather than the mesh section that found this — that geometry is in a private repo and this one is public. Closes #322.Task
VortexStepMethod.jl-320