Skip to content

write_section_aero writes an all-NaN deflected .dat, and read_section_ - #301

Merged
1-Bart-1 merged 7 commits into
mainfrom
agent/300-write-section-aero-writes-an-all-nan-def
Sep 12, 2026
Merged

write_section_aero writes an all-NaN deflected .dat, and read_section_#301
1-Bart-1 merged 7 commits into
mainfrom
agent/300-write-section-aero-writes-an-all-nan-def

Conversation

@1-Bort-1

@1-Bort-1 1-Bort-1 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

TL;DR

write_section_aero no longer writes a deflection whose contour is all NaN, and the two .dat readers that disagreed about NaN rows are now one strict reader. A deflection the 2D solver converged at no angle used to reach a loaded Wing as a NaN airfoil shape at every deflection, 0° included, with no error and no warning.

What was wrong

generate_airfoil_aero writes a contour column only from a solution whose node count matches the grid's modal one, and XFoilSolver returns empty node arrays for a non-converged angle — so a deflection that converged nowhere leaves x[:, jd] and y[:, jd] at their fill(NaN, …) initial value. write_section_aero wrote that column out as airfoils/<id>_d<tag>.dat full of NaN NaN rows, three lines above a guard whose comment already names the hazard for {dat_prefix}.dat.

Then the readers disagreed. read_dat_coordinates (submodule) kept only rows starting with a digit, - or ., so an all-NaN file read back as zero points and its caller crashed — that is the CI failure on #299, argmin on an empty x inside normalize_airfoil. read_dat (main package) had no such filter and tryparse(Float64, "NaN") succeeds, so read_section_aero got a full-length NaN vector, the lengths matched, and the NaNs landed in x_interp/y_interp. Linear interpolation between a finite node and a NaN node is NaN everywhere, which is why the contour came out NaN at 0° as well. Ten lines reproduce it with no solver at all, a SectionAero whose 1° column is NaN:

wrote _d1.dat: isfile=true
section | NaN NaN | NaN NaN
read_dat_coordinates: 0 points
read_section_aero: SectionAero{…}
  contour at delta=0.0: x=[NaN, NaN, NaN, NaN, NaN]
  contour at delta=1.0: x=[NaN, NaN, NaN, NaN, NaN]

What I changed

write_section_aero hoists the finite(jd) helper it already had above the deflection loop and skips a column with no finite contour, so no all-NaN .dat is ever written and isfile stays the honest test for a generated deflection — which is what the Makie audit plot already assumes, warning and drawing nothing for a deflection it finds no file for.

The duplicate reader is gone rather than aligned. read_dat_coordinates moves into src/section_aero.jl beside read_node_table and read_section_aero, since loading lives in the main package and writing in AirfoilAero; AirfoilAero imports and re-exports it, so every existing caller and the using VortexStepMethod.AirfoilAero: read_dat_coordinates in the tests are unchanged. It now drops a row by value (isfinite) instead of by first character, which is what the first-character filter was approximating: that filter skipped NaN NaN only because N is not a digit, and let 0.5 NaN through. I searched for further copies before deleting the second one and found a third — read_dat_coords in test/airfoil_aero/test_airfoil_aero.jl, a test-local reader of the same format — and deleted it too, in the file this PR already changes.

read_section_aero is the last link: a deflected contour that is missing, or that does not hold exactly its Cp table's node count, now warns and returns nothing instead of quietly filling the interpolants. nothing is a state the loader already returns and add_section! already takes, so an airfoil with a broken contour set loses its surface aero and keeps its polar, rather than poisoning the whole wing's geometry.

The third item, and a better fix underneath it

Should generate_airfoils fail an id outright when a deflection converged at no angle, the way all(isnan, clvals) already fails one for column 1? I would, and for the same reason: the coefficients of such a column survive only because fill_node_nans! interpolated them from the neighbouring deflections, so what the id offers at that delta is a fabricated polar and, after this PR, no surface data at all. reuse_valid_airfoils=true then maps those sections to the nearest id that solved everywhere — a visible, documented degradation — and reuse_valid_airfoils=false errors, which is what a caller who turned the fallback off asked to hear. The push-back is that it costs a whole airfoil for one fussy deflection, and #291 says that fixture's convergence moves between runs.

There is a cheaper fix underneath it that I would rather see: the contour does not depend on the solver converging at all. It is deform_section's own output, handed in, and NeuralFoilSolver already carries it straight through (s.x = def.x). I measured what XFoil gives back at repanel=false — 239 nodes against def's 239, max|dx| = max|dy| = 0.0 at both 0° and 3° — so generate_airfoil_aero could take x/y from def whenever its node count matches the modal one and no geometry column would ever be NaN, for either backend. That does not subsume this PR: a converged deflection whose node count is not the modal one still leaves a NaN column, so the writer's guard stays the backstop. Say the word and I will open it as its own task.

This is the fix for the red Test end-user and developer setup check that #299 carried, and that check is green here: with no all-NaN _d1.dat written, generated_slices takes its isfile branch and warns instead of crashing in normalize_airfoil. #295 names the same guard but is about a degenerate finite fit, and neither fix covers the other.

The one red job is not this diff

Julia 1.12 - windows-latest - x64 on 5e78ab6 is #287's flake, and main settles it rather than my say-so: twenty minutes after this run, main's own push CI for the #308 merge (run 34696079829) failed the identical assertion in Julia 1.12 - ubuntu at relative_error(jac_fd, jac_fwd) = 0.0420681631196805, against this branch's 0.04206403446037785 at test/solver/test_forwarddiff.jl:87. Same test, same ~0.042 branch of the bimodal jump #287 records, on a commit carrying none of these seven files. Which cell fires moves run to run and OS to OS: windows red and 1.12 ubuntu green here, 1.12 ubuntu red and windows green on main, and on the earlier 3c4fd7e both 1.12 ubuntu cells at 0.03999947 and 0.04001102 with windows green. It is a bug in the test — a 1e-4 bound over a discrete interpolant jump — and #287 owns it.

So the failing check is not evidence about this diff, and I ruled the diff out by measurement rather than by argument. test_forwarddiff builds its wing from a generated directory, which does go through the reader this PR rewrites, so I deleted that fixture and regenerated it through this branch's writer: 16 .dat files, not one with a NaN row, and the strict reader returns coordinates identical to the lenient one it replaces on all 16. The change cannot alter what that test loads. On that fresh fixture the file is 7/7 green with norm(jac_fwd) = 3.2971127281317214 and rel_err = 5.22e-8 — 1900x inside the bound, and 800000x below what CI saw.

Merging main in

main has since taken #294, #299, #306, #310 and #312, and v5.1.0 went out. Only CHANGELOG.md conflicted, both sides having added to the same ### Fixed block: my entry moves to ## Unreleased, where it belongs now that v5.1.0 shipped without it, and both of main's entries stay verbatim.

One conflict was semantic and the textual merge hid it. #294 added @testset "generate_airfoils fits the wrapped contour it is handed" at the end of test/airfoil_aero/test_airfoil_aero.jl, and it calls read_dat_coords — the test-local third reader this PR deletes. The merged file was clean to git and UndefVarError to Julia. Both calls now go through read_dat_coordinates, which is the point of the deletion; the two files it reads are finite, so the strict reader returns the same coordinates.

main's #312 guards the other end of the same bug — the audit plot skipping a deflected .dat that reads back empty — and the two do not overlap: #312 protects a directory already generated with an all-NaN .dat, this stops one being written. Its "Audit slices (Makie)" testset writes its blank file by hand, so it still exercises that guard with this writer in place, and passes 7/7 here.

Verification

  • Reproduced first: contour at delta=0.0: x=[NaN, NaN, NaN, NaN, NaN] from a SectionAero whose only bad column is 1°, written and read back through the public writer and loader
  • test/airfoil_aero/test_airfoil_aero.jl — new testset "a deflection with no contour is neither written nor loaded as NaN" 2 passed / 3 failed before the change (the .dat was written, the load returned a SectionAero), 7/7 after
  • Re-run against the merged tree (juliaserver, examples env): airfoil_aero/test_airfoil_aero.jl (87 over 11 testsets, Fit the contour generate_airfoils is handed, not a second wrap of it #294's new one among them at 2/2 and this PR's at 7/7), airfoil_aero/test_live_polar.jl (93), obj_adapter/test_obj_adapter.jl (52, the generate → Wing(yaml) round trip), solver/test_backend_comparison.jl (15), yaml_geometry/test_yaml_geometry.jl (178), plotting/test_plotting.jl (58 + 19 + Setup Test is failing #312's 6 and 7, the other caller of the moved reader), Aqua.jl (10) — no failures
  • Docs build clean against the merged tree (docs/make.jl, only the pre-existing size_threshold_warn notices on private_functions.md and functions.md) · REUSE: n/a, no bin/reuse_lint · merged up to origin/main (8b0c6bc)
  • GitHub CI on 5e78ab6: six of seven checks green — Documentation, Julia 1.11 - ubuntu, Julia 1.12 - ubuntu, Julia 1.12 - macOS-aarch64, codecov/patch and Test end-user and developer setup. Julia 1.12 - windows red on test_forwarddiff.jl's POLAR_MATRICES check flakes at ~4%, and the oper #287's flake alone (0.04206403446037785), with this PR's testset 7/7 inside it, and main red on that same assertion at 0.0420681631196805 twenty minutes later. Local CI mirror (agent ci-local, one matrix cell) on the merged tree: PASS in 13m
  • test/solver/test_forwarddiff.jl on this branch, fixture deleted and regenerated through the new writer: 7/7 green, norm(jac_fwd) = 3.2971127281317214, rel_err = 5.22e-8 against the 1e-4 bound (juliaserver, examples env)
  • Every test/generated/ fixture deleted and rebuilt through this branch's writer, then the two suites that consume them re-run, because the copies my earlier runs used were written before this branch took Fit the contour generate_airfoils is handed, not a second wrap of it #294: obj_adapter/test_obj_adapter.jl 52/52 and plotting/test_plotting.jl 58 + 19 + 6 + 7 — same counts as against the cached fixtures, so nothing in this PR's evidence rested on the stale contour
  • Risk: read_section_aero returning nothing where it used to throw a SystemError for a deflected contour it cannot open. Every generated directory on disk today with an all-NaN _d1.dat now warns and loads without surface aero instead of loading a NaN one, which is the intent — but a hand-made directory whose deflection tag does not match delta_suffix degrades where it used to fail loudly.

Scope

+70 / −65 across 7 files against the merge base (8b0c6bc), of which 20 deleted lines are the two duplicate .dat readers and 23 are the moved one. No stack.

Found on the way, not fixed here: test/generated/'s fixture path is keyed on hash((n_sections, alpha_range, delta_range)) and nothing about the code that fills it (test/test_data_utils.jl:46-49), so a box whose cache predates a contour-generation change keeps testing the old contour. It cost me a wrong number: my first test_forwarddiff run on this merged tree reported norm(jac_fwd) = 2.2059, #294's pre-fix doubly-wrapped value, from a directory generated the day before; regenerating gave 3.29711. CI never sees it, checking out fresh each run. Keying the path on the generator's version, or stamping the config into the directory, is a change to shared test infrastructure that no test here needs — its own task, and worth one to whoever picks up #287, whose next step is reproducing that fixture.

Also found, not fixed: test/test_body_aerodynamics.jl, test/test_solver.jl and test/test_yaml_geometry.jl are tracked and zero bytes — leftovers of the move into test/<area>/, which nothing includes. includeing one silently does nothing, which is how I met them. Three file deletions in a diff that has no other reason to open them is a cleanup: PR; say the word and I will open it.

Closes #300 · task VortexStepMethod.jl-300

A deflection XFoil converged at no angle left generate_airfoil_aero's contour
column at its NaN initial value; write_section_aero wrote it out as a .dat of
`NaN NaN` rows, and read_section_aero's own tolerant reader parsed those back
into the wing's contour interpolants, so every deflection interpolated to a NaN
airfoil shape.

write_section_aero now skips a deflection column with no finite contour, and the
two .dat readers become one: the strict read_dat_coordinates moves into the main
package beside the other loaders, drops NaN rows by value rather than by first
character, and AirfoilAero imports it. read_section_aero warns and returns
nothing when a contour does not hold its Cp table's nodes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013kHEhSNd6mzegjH4o8bjv9
@1-Bort-1 1-Bort-1 added agent:running Agent task state agent:ci Agent task state agent:queued Agent task state and removed agent:running Agent task state agent:ci Agent task state agent:queued Agent task state labels Sep 11, 2026
@1-Bort-1

1-Bort-1 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Local full suite: PASS (12 min, Julia 1.12.7, one cell of the matrix)

@1-Bort-1 1-Bort-1 added agent:ci Agent task state agent:waiting-human Agent task state and removed agent:running Agent task state agent:ci Agent task state labels Sep 11, 2026
@codecov

codecov Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

…tes-an-all-nan-def' into agent/300-write-section-aero-writes-an-all-nan-def
@1-Bort-1

Copy link
Copy Markdown
Contributor Author

Answering the question from the thread — what works, what does not, is deflected wrapping working, and did the continuous wrap regress anything. Measured on this branch; every number below is from a run I did today, not from a previous session.

Did anything regress going continuous?

Yes — two things, both found and fixed on the wrap/rolling-ball branch before #262 ever reached main, and both now have a test.

ecc852c is a stated regression, in its own words: "The rolling-ball wrap floored clearance at min_clearance for every convex corner, so a closed airfoil wrapped at clearance=0 grew a round trailing-edge cap of min_clearance radius. XFoil flags the contour sharp (first point equals last) yet the geometry is blunt, and the blunt TE shifts the Kutta point: cl came out ±0.486 instead of ±0.434 and the viscous march failed to converge." That is exactly the XFoil-convergence "Known limitation — not ready to merge" that #262's own body carried. It is now pinned by test/airfoil_aero/test_airfoil_aero.jl:62-64.

1e13d92 ("Offset the rolling ball's contact polygon instead of its rim") rewrote the offset geometry and added the testset "Shrink-wrap at zero clearance lands on a clean airfoil" (:67), asserting every wrap node lies within 1e-9 of the input contour. Its commit body has no prose, so it is not labelled a regression fix, but it is the same class.

git log 536a67f..HEAD -- src/airfoil_aero/shrink_wrap.jl is exactly those two commits and nothing else, and no issue or PR since the merge reports a wrap-geometry regression.

The caveat I would not want buried: #293 (the double wrap) may not be independent of the rewrite. The mechanism that issue names — smoothed_curvature dividing turn across ~1e-16 spans — is code that arrived with 536a67f. The double call predates it; whether the old grid-sampled wrap absorbed a second pass I cannot tell you, because that code is deleted and I did not revive it to measure. So: the second wrap is an old mistake, the damage it now does may be new. I measured the damage itself on test/airfoil_aero/data/test_airfoil.dat:

                                 n      min_span     |w|max     |y_refit|max
raw slice                        45     0.00816      0.59413    0.09644
wrapped once                     239    0.00117      0.52916    0.10068
wrapped TWICE (#293's path)      239    0.00101      2.7234     0.10532
deform 10 deg off wrapped once   239    0.000415     1.239      0.10454
deform 10 deg off wrapped twice  239    0.000916     3.2222     0.10678

The second wrap multiplies the Kulfan weights by 5.1x on this small fixture while the shape still looks fine (|y| barely moves) — which is why it went unnoticed — and deflection compounds it, 1.24 to 3.22. On the SK100 the same mechanism reaches 1e4. #294 is the fix and is agent:blocked.

Is shrink wrap at delta != 0 working?

Structurally yes, and I could not break it. Every deflection gives a finite, closed, 239-node contour — the node count is delta-independent, so the (alpha, delta) grid stays rectangular:

crease_frac=0.9, so an ideal rigid rotation drops the TE by 0.1*sin(delta)
delta[deg]  n_nodes  finite  closed  TE_drop     ideal       ratio
-10.0       239      true    true    -0.01178    -0.01736    0.678
 -5.0       239      true    true    -0.007596   -0.008716   0.872
 -2.5       239      true    true    -0.003768   -0.004362   0.864
  0.0       239      true    true     5.299e-5    0.0         -
  2.5       239      true    true     0.003869    0.004362    0.887
  5.0       239      true    true     0.007692    0.008716    0.883
 10.0       239      true    true     0.01362     0.01736     0.784

The deflection goes the right way, scales with the angle, and is symmetric in sign. What it is not is equal to a rigid rotation: the TE lands 12% short at small angles and 22–32% short at ±10°. Some of that is certainly my crude datum (mean y of the nodes near x = 0.9) and some is the re-wrap rounding the crease, which is deliberate. But nobody knows which, because no test has ever checked it. There is no test anywhere in test/ that compares a deflected contour against a reference shape — the closest, test_airfoil_aero.jl:220-234, asserts only that camber rises above 0.02 and the shape stays roughly on the unit chord, which a deflection of half or double the requested angle would also pass. That is precisely the coverage #296 asked for; it was closed by folding into #294's review, and #294 is blocked, so the gap is still open.

The good news on the other half of #296: its 10_d10.dat pathology (lower surface collapsing to a constant min_clearance/chord) is not generic. On a healthy wrapped slice the 10° lower surface has 237 distinct values over 239 nodes, spanning -0.083 to 0.105. It is a symptom of the blown-up sections, not of deflection.

So: what works, what does not

Works. The continuous wrap itself, at every delta, including zero clearance where it now hugs the input to 1e-9. Deflection geometry: finite, closed, delta-independent node count, right sign and right order of magnitude. Both solver backends at delta != 0 — NeuralFoil and XFoil agree within 0.03 on cl/cd/cm across the deflected grid (test_backend_comparison.jl:8,71). Polar-matrix delta axes round-trip through write/read in both CSV and Arrow.

Does not work, none of it the wrap's geometry:

what owner state
all-NaN deflected .dat written, and silently loaded as a NaN contour at every delta this PR fixed here
obj_to_yaml wraps, then generate_airfoils wraps again — Kulfan fit degenerates, .dat at 1e4 scale for ~9 of 45 SK100 sections #293 / PR #294 agent:blocked
the Kulfan least-squares fit has no rank guard, which is why the double wrap explodes rather than merely drifts #295 open, unstarted
generated_slices rolls its own _d<deg> tag instead of delta_suffix, so plots miss negative and fractional deflections #298 / PR #299 agent:waiting-human
no test asserts deflected contour geometry against a reference was #296, folded into #294 blocked with #294
the generated ram-air fixture is not reproducible run to run #291 open, unstarted

One more thing the rewrite left behind, too small for its own task but I will not walk past it: src/airfoil_aero/airfoil_solvers/common.jl:99-101 still describes the old behaviour — it says the re-wrap "hugs the deflected shape at min_clearance" and "bridges the crease with a min_concave_radius fillet", but ecc852c made a closed loop use its true clearance (so clearance=0 hugs exactly, no floor) and 1e13d92 made narrow concavities bridge straight instead of filleting. Separately, deform_section hardcodes ShrinkWrap(clearance=0.0) in its signature and discards the caller's configured wrap_method, so the mesh: block's clearance never reaches the per-deflection re-wrap. I would put both in one cleanup: PR.

What I would do next, in order

  1. Merge this PR — it stops the silent NaN contour, which is the one failure with no symptom.
  2. Unblock Fit the contour generate_airfoils is handed, not a second wrap of it #294; it is the one with a real scale error behind it, and it carries No test runs generate_airfoils with a delta_range over a wrapped slice, so the per-deflection .dat files are unchecked #296's delta-grid test.
  3. The Kulfan least-squares fit has no rank guard, so a crowded resample writes an airfoil at 1e4 scale with a normal residual #295 under it, so the fit refuses rather than returns nonsense.
  4. Then the deflected-geometry reference test, which is the only reason I cannot answer "is the 12% shortfall real?" today.

CI on this branch

Both red jobs earlier were pre-existing and are now fixed on main: the 1.12 coverage job was #287's rel_err = 0.04001 flake (#292, merged) and the 1.11 one was #288's 30-minute job timeout after a green test step (#289, merged). I merged origin/main in, so this branch now carries both; CI is re-running on 3c4fd7e.

@1-Bart-1

Copy link
Copy Markdown
Member

Resolve the conflicts

@1-Bort-1 1-Bort-1 added agent:queued Agent task state agent:running Agent task state and removed agent:waiting-human Agent task state agent:queued Agent task state labels Sep 12, 2026
1-Bort-1 and others added 2 commits September 12, 2026 14:54
…-def

Only CHANGELOG.md conflicted: v5.1.0 shipped and #312's audit-plot entry landed
in the Fixed block this branch's entry sat in. The entry moves to Unreleased.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLm7oza8A7H8xtGQP2hp7Z
The merge was textually clean but semantically not: main's
"generate_airfoils fits the wrapped contour it is handed" calls the
test-local read_dat_coords this branch deleted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WLm7oza8A7H8xtGQP2hp7Z
@1-Bort-1

Copy link
Copy Markdown
Contributor Author

Resolved in 0370655 and 5e78ab6; #301 is MERGEABLE again.

@1-Bort-1 1-Bort-1 added agent:ci Agent task state agent:queued Agent task state agent:running Agent task state and removed agent:running Agent task state agent:ci Agent task state agent:queued Agent task state labels Sep 12, 2026
@1-Bort-1 1-Bort-1 added agent:review Agent task state and removed agent:ci Agent task state labels Sep 12, 2026
@1-Bart-1
1-Bart-1 enabled auto-merge September 12, 2026 15:15
@1-Bart-1
1-Bart-1 merged commit 8a4d177 into main Sep 12, 2026
7 checks passed
@1-Bart-1
1-Bart-1 deleted the agent/300-write-section-aero-writes-an-all-nan-def branch September 12, 2026 15:16
@1-Bort-1 1-Bort-1 added agent:done Agent task state and removed agent:review Agent task state labels Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:done Agent task state

Projects

None yet

Development

Successfully merging this pull request may close these issues.

write_section_aero writes an all-NaN deflected .dat, and read_section_

2 participants