[1-Bort-1]

TL;DR

get_lower_upper now returns the lowest and highest point where the contour crosses the vertical line x = crease_frac. It used to return the nearest point below y = 0 and the nearest point above it. On a cambered section that lower point sits near the chord line, and the upper point can be on the lower surface. deform_section pivots the flap about these heights, so negative deflections turned about the wrong point, and sometimes positive ones did too.

What was wrong

On a V3_25 canopy section, wrapped with ShrinkWrap() as obj_to_yaml does, the whole lower surface sits above y = 0 at the crease. The nearest point with y < 0 is then a stray point near the chord line. And the nearest point with y ≥ 0 is whichever surface happens to have a node closer to x = 0.9. On mid-span sections that was the lower surface. So the thread's diagnosis is right but the damage is wider: the upper height, which is the delta > 0 hinge, was also wrong on 5 of the 18 sections.

Hinge heights at crease_frac = 0.9, V3_25 `perpendicular_sections(…, 18; n_bins=100)`, wrapped

section	crossings (upper, lower)	old (lower, upper)
5	0.01660, 0.00439	−0.00047, 0.00428
7	0.01805, 0.00598	−0.00004, 0.01819
9	0.01887, 0.00676	−0.00081, 0.00717
10	0.01888, 0.00679	−0.00162, 0.00721
12	0.01812, 0.00609	−0.00059, 0.00614

Bold marks an "upper" height that is the lower surface. After the change the function returns the crossings on all 18 sections.

What changed

The function walks the segments between consecutive points and interpolates y wherever a segment crosses x = crease_frac. The test is half-open, so a vertex lying exactly on the line counts once. The closing segment, from the last point back to the first, is not tested, and the docstring says so. I searched src/, ext/ and test/ for an existing line-crossing or contour-interpolation helper (crossing, intersect, interp, crease_frac) and found none to reuse. It returns the minimum and maximum crossing. Fewer than two crossings throw an ArgumentError. That only happens for an open curve, such as an unwrapped slice, and before this change such a curve silently got a hinge at y = 0. Every caller in src/, ext/ and test/ passes a closed contour.

Effect on polars

The change is small. On section 9 the lower hinge moves up by 0.0076 c and the upper one by 0.0117 c. With NeuralFoil medium at Re = 1e6 and α = 0/5/10°:

• δ = −10°: cl 0.0416/0.5603/1.1590 → 0.0405/0.5581/1.1566
• δ = +10°: cl 0.3309/0.9275/1.6920 → 0.3314/0.9282/1.6925
• cd moves by at most 1.5e-4 and cm by at most 2e-4.

The "old" row comes from a verbatim copy of main's get_lower_upper, fed through the same deform, wrap and fit steps in the same session. I left out a before/after figure: a 0.0076 c hinge shift doesn't show at section scale.

Where I'd push back

turn_trailing_edge!'s legacy branch, when thickness_frac === nothing, also reads these heights. Only its own test calls it. It now gets real surface heights too, and that test passes unchanged. That branch still sorts points into upper and lower surface by the sign of y, which is the assumption this PR removes from get_lower_upper. Its turn_distance is now the real thickness at the crease, which is smaller than the old span. On a thin or strongly cambered section smooth_idx can come back empty, and then minimum(smooth_idx) throws. I left the branch alone because no caller outside its own test reaches it. If it is worth keeping, it needs the same crossing-based surface split. Otherwise it can be deleted.

#365 edits deform_section and the same test file in a different place. Whichever merges second may need a trivial CHANGELOG.md / test-file merge. There is no overlap in logic.

Verification

• Reproduced first: V3_25 section 9, old (lower, upper) = (−0.00081, 0.00717) against crossings (0.00676, 0.01887).
• New testset "get_lower_upper takes both surfaces at the crease chord station" was red before the fix: old (−0.00025, 0.04049) against the true (0.02141, 0.04039), and no ArgumentError. Green after (4/4), and again after 87d30de along with the rest of test_airfoil_aero.
• test_airfoil_aero, test_backend_comparison, test_obj_adapter: 163 pass. test_forwarddiff, test_results, test_wake, test_refinement_validation: 58 pass, with test/generated/ rebuilt. All run through juliaserver.
• Local CI (agent ci-local, Julia 1.13): PASS in 9 min, exit 0.
• GitHub CI on the merge commit f0765eb: PASS on every job, Documentation included. The one earlier red, test_forwarddiff.jl:100 on 87d30de (relative_error = 0.046, Julia 1.12 ubuntu), is the test_forwarddiff.jl's POLAR_MATRICES check flakes at ~4%, and the oper #287 flake. I ruled this diff out by measurement: the fixture that assertion builds goes through deform_section. Rebuilt under this branch's src and under origin/main's, its .dat files, polar matrices and both linearize Jacobians were bit-identical.
• Docs build: the CI Documentation job passes. REUSE lint: n/a (the repo has no bin/reuse_lint). Merged with origin/main in f0765eb.
• Risk: a caller passing an open curve now gets an ArgumentError where it used to get a silent hinge at y = 0.

Scope

+35 / −21 across 3 files, 87d30de being the review round (error message and docstring only). The function body is smaller than the one it replaces. The rest is a 15-line test and a changelog entry.

Closes #364 · task VortexStepMethod.jl-364

[1-Bort-1]

Independent review (advisory)

Verdict: APPROVE WITH COMMENTS · 1 inline, 0 off the diff

Good

• The fix does what the card says: get_lower_upper now interpolates y at every segment crossing of x = crease_frac and returns the lowest and highest, so the hinge is on the real surfaces (checked polar_gen.jl:71-84)
• The half-open test min <= c < max is right on the edge cases: a vertex on the line counts once in either walk direction, a touching extremum counts zero times, and a vertical segment can never divide by zero (checked by hand on both surface orderings)
• No duplication: the only other crossing code, obj_slice.jl:294, finds leading and trailing edges of a 3D slice, not a height at a chord station
• The only callers are deform_section (common.jl:111) and the legacy test; both pass closed contours, so the new ArgumentError changes nothing on any existing path
• The function is shorter than the one it replaces, the docstring states what it does with no history, and the file's stray trailing blank line went with it
• The new test is named after the behaviour, uses a cambered section whose lower surface is above y = 0 (the exact failure), states that precondition as an assertion, and covers the open-curve throw
• The CHANGELOG entry is compact, user-facing, and says that flapped polars shift
• The card's before/after table and polar deltas, made against a verbatim copy of main in the same session, make the size of the effect judgeable without rerunning anything

Not good

• src/airfoil_aero/polar_gen.jl:82 — The message says a closed contour crosses 'twice', but the check accepts any count of two or more, and a wavy wrapped canopy can cross four times. A caller who hits the error gets told the wrong rule; 'at least twice' states what the code enforces.
• turn_trailing_edge!'s legacy branch (polar_gen.jl:30-33) still sorts points into upper and lower surface by the sign of y, the same wrong assumption this PR removes; the card mentions only that the branch reads the heights, so file it as a note or issue
• turn_distance in that legacy branch is now the true thickness at the crease, much smaller than the old span; on a thin section smooth_idx could come back empty, and minimum(smooth_idx) would then throw where it didn't before
• The card doesn't say what was searched before writing the crossing loop (rubric §2); the search turns up nothing to reuse, but the card should say so
• The segment from the last point back to the first is never tested; harmless for crease_frac < 1 on wrapped contours, but the docstring says 'closed contour' without saying whether the first point must be repeated at the end

_{claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.}

[1-Bort-1]

MINOR: The message says a closed contour crosses 'twice', but the check accepts any count of two or more, and a wavy wrapped canopy can cross four times. A caller who hits the error gets told the wrong rule; 'at least twice' states what the code enforces.

[1-Bort-1]

Fixed in 87d30de: the message now says 'at least twice'. The docstring also says the segment from the last point back to the first is not tested. The card now says what I searched for, and has a note on the legacy branch's y-sign split and the empty smooth_idx case.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!