Each panel takes its own wing's spanwise direction in calc_forces! and calculate_results - #367
Conversation
… calculate_results Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…irection Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
- The fix matches the card: both loops go wing →
panel_rangeand usewing.spanwise_direction. The body sums still usewings[1]'s span, as the card says (solver.jl:445-479, body_aerodynamics.jl:950-1001). - For a single wing,
calculate_span(body_aero.wings, reference_spanwise)gives the same number as the oldwings[1].span.spanis a virtual property that callscalculate_span(w)(wing_geometry.jl:1755), and that is nowcalculate_span((wing,), wing.spanwise_direction). panel_rangereplaces the hand-keptpanel_idxcounter and theelsebranch that only skipped panels. The unrefined-section accumulation (solver.jl:526-573) now indexes panels the same way as the force loop.- The body-sum decomposition still uses the reference frame, so a fin's lift still shows up in the body's
side. The test pins this:side ≈ Fyfor the pair, where the fin's lift points along −y. - The test reproduces the bug. The rotated wing's F, cl, cd and cs must equal the solo wing's rotated results, and the card reports 5 of 9 failing on main's
src. - The card's claim that
src/gets shorter checks out. The only otherwings[1]read is body_aerodynamics.jl:517, in the elliptical initial distribution, which throws for more than one wing and is tracked as #357. prescribed_va_directionsandpanel_rangeare documented inprivate_functions.md. The docstring of the newcalculate_spanmethod is shared with the existing one.
Not good
test/solver/test_wing_directions.jl:11—rectangular_wingis a fourth near-copy of the flat two-section test wing, next toinviscid_wing(test_body_aerodynamics.jl:18) andflat_plate_wing(test_solver.jl:244). It differs only in rotation, offset and polar. Extending one of those with keyword arguments would keep a single builder (§2).src/body_aerodynamics.jl:944— The guard computesnorm(cross(va_ref_vec, reference_spanwise))and then throws that result away. Line 947 computes the same cross product and normalises it again insideprescribed_va_directions. The degeneracy check and the direction now have two sources three lines apart.- body_aerodynamics.jl:976 introduces
panel_va, but line 985 still readsdot3(panel.va, va_ref_unit). The same loop reads the vector two ways. - The per-panel
prescribed_va_directions(panel_va, spanwise_unit)has no degeneracy guard. A panel whose wing lies along its inflow gets NaNcl/cs_distributionwithout any error, where the reference directions throw. - In test_wing_directions.jl, the aspect-ratio testset passes a literal
4for the section count, while the testset above uses2length(body_aero.wings)for the same thing. - The card lists what was searched in
src/but not which test helpers were checked before writing a new wing builder (§2 'say what you searched for').
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| A rectangular wing spanning `y`, with every point turned by `rotation` and moved by | ||
| `offset`, and its spanwise direction turned with it. | ||
| """ | ||
| function rectangular_wing(rotation, offset) |
There was a problem hiding this comment.
MINOR: rectangular_wing is a fourth near-copy of the flat two-section test wing, next to inviscid_wing (test_body_aerodynamics.jl:18) and flat_plate_wing (test_solver.jl:244). It differs only in rotation, offset and polar. Extending one of those with keyword arguments would keep a single builder (§2).
There was a problem hiding this comment.
Not here: inviscid_wing, flat_plate_wing and build_flat_wing are each local to a test file this PR does not open, so merging them into one builder in test_data_utils.jl means rewriting three other test files. That belongs in its own cleanup: PR. The card now says what I searched.
| cross3!(dir_lift_ref, va_ref_vec, spanwise_direction) | ||
| dir_lift_ref_norm = norm3(dir_lift_ref) | ||
| dir_lift_ref_norm > 0.0 || throw(ArgumentError( | ||
| norm(cross(va_ref_vec, reference_spanwise)) > 0.0 || throw(ArgumentError( |
There was a problem hiding this comment.
MINOR: The guard computes norm(cross(va_ref_vec, reference_spanwise)) and then throws that result away. Line 947 computes the same cross product and normalises it again inside prescribed_va_directions. The degeneracy check and the direction now have two sources three lines apart.
There was a problem hiding this comment.
Fixed in f3c2339: the guard now checks the reference_dirs it builds, so the cross product is taken once.
Take the panel's inflow from panel_va throughout the loop, and size the aspect-ratio test's solver from its wing count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Local full suite: PASS (5 min, Julia 1.13.0, one cell of the matrix) |
…e-th Keeps the per-wing loop in calculate_results over main's Panel.va -> va_vec rename, and both Fixed entries in the changelog. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#366 added spanwise_extent(wing) beside this branch's calculate_span(wings, direction); both projected the sections on a span axis. spanwise_extent now takes the wings and a direction, and calculate_span is its hi - lo. The elliptic circulation loop walks the wings with panel_range. CHANGELOG keeps both Fixed entries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
I don't fully understand how this works yet. Some figures in the PR body would help. And more explanation on what the spanwise directions are, and how the lift direction is now determined with multiple wings, and what spanwise becomes with multiple wings.
There was a problem hiding this comment.
Rewrote the PR description: a new section, 'How the directions are built', with before/after figures of a wing plus a deflected fin, rendered from main's src and d066ca6. In short: each Wing keeps its own spanwise_direction. A panel's lift comes from its own section geometry and never used it. The span direction sets the panel's drag, cross(spanwise, lift), and on main a fin's drag came out along -z, 44 N of lost lift in the figure. The body's lift/side sums and wing_span stay in the first wing's frame, as before.
TL;DR
calc_forces!andcalculate_resultsnow use each panel's own wing'sspanwise_directionwhere every panel usedwings[1]'s. That direction sets the panel's drag direction, and on a fin next to a wingmainturned the fin's drag into lift.solvealso takeswing_spanandaspect_ratio_projectedfrom all wings. Both gave wrong numbers with no error, and #355 passes the same directions on tolinearize's Jacobians.How the directions are built
spanwise_directionis a field of eachWing, a unit vector along its span given by the user (default[0, 1, 0]). A horizontal wing spans y; a vertical fin should be built withspanwise_direction = [0, 0, 1]. Each wing keeps its own; nothing here merges them.For each panel, three directions come out of
panel_force_directions:cross(inflow, y_airf), from the panel's geometry). It never usedspanwise_direction, so it was already right per panel onmain: up on a wing, sideways on a fin.cross(spanwise, lift). This is where the span direction enters. With the panel's own span it points downstream. Onmaina fin panel crossed the wing's y with a lift that is itself along ±y, and what was left pointed along ∓z.calculate_results, thecl/cd/cs_distributionsplit of each panel's force uses the same span: lift normal to inflow and span, side normal to lift and inflow.The body-level numbers take one frame, the first wing's: the body's
lift/drag/sidesums project every panel's force onto the lift and side directions ofwings[1]'s span, andwing_span/aspect_ratio_projectedmeasure all wings along that span. For a wing with a fin,liftis the vertical force andsidethe lateral one, fin included.The figures: a 6 m × 1.5 m wing at 4° plus a fin of the same size 6 m behind it, deflected 5°, symmetric polar with cd = 0.02, va = 20 m/s along x. Left: looking downstream, the span direction each wing's panels use (green) and the panel lift (blue), the same in both. Right: from the side, the panel drag direction (red), taken from the solved forces as
normalize(F − L·dir_lift). The fin's drag, 0.02 × 245 Pa × 9 m² = 44 N, goes into −Fz onmaininstead of Fx.The test
The test builds a wing along y and an identical wing turned 90° about x, placed 1000 m away along y so the two do not induce on each other. Inflow is along x, which the rotation leaves unchanged. On
main, the turned wing's panels come out with Fx = 1.52 N where the one-wing result, rotated, gives 5.93 N. Itscl_distributionis −0.020 against 0.176–0.283, and itscs_distributionis −0.18 to −0.29 against 0. The lift and drag magnitudes (lift_dist,drag_dist) already agreed; only the directions were wrong.What changed
calc_forces!andcalculate_resultsloop wing →panel_range(body_aero, wing_idx)and usewing.spanwise_direction.panel_rangeis new, next tounrefined_section_range, and both are now onesumover the preceding wings. The unrefined-section accumulation incalc_forces!uses it too, which drops its hand-keptpanel_idxcounter and theelsebranch that skipped wings.calculate_resultsthe prescribed-va split is one helper,prescribed_va_directions(va, spanwise). It is used three times: for the reference directions, and twice per panel. The per-panelcl/cd/cs_distributionuse the panel's own wing. The body'slift/drag/sidesums still decompose each panel in the reference frame (wings[1]'s span). Decomposing a fin panel in its own frame and then projecting lift onto lift and side onto side would drop the fin's side force from the body'sside. With inflow along x, the test checkslift == Fz,side == Fyanddrag == Fxon the two-wing body. For a single wing all three sums are the same arithmetic as before.calculate_span(wings, spanwise_direction): a new method of the exported function, giving the extent of all wings' sections along a direction. It ishi - loofspanwise_extent(wings, spanwise_direction), the helper Start ELLIPTIC circulation on each wing's own ellipse, so multi-wing bodies solve #366 added for one wing, now taking several wings, so the projection exists once. Start ELLIPTIC circulation on each wing's own ellipse, so multi-wing bodies solve #366's elliptic loop walks the wings withpanel_rangeinstead of its own offset counter.wing_spanandaspect_ratio_projectedinsolvenow usecalculate_spanalongwings[1]'s span. Two touching 6 m wings give 12 m and AR 8; onmainthey give 6 m and AR 2.Where I would push back
wings[1]'s: it sets the body's lift/side axes and now its span. The first wing has to be the main wing. Nothing checks this, but the only alternative is a body-frame setting nobody has asked for.projected_areaalready does. Two wings far apart count the gap as span too, just as a planform would.cl/cs_distributionof a panel whose inflow runs along its own wing's span come out NaN, with no error; the reference directions throw on the same case.mainhas the same gap withwings[1]'s span. Turning that NaN into an error is a behaviour change, so it is not in here.stability_derivatives(new from stability_derivatives gives roll, pitch and yaw rate derivatives about solver.reference_point #356) scalesdpanddrbyb = wings[1].span. For a wing with a fin that is the right span, but for wings side by side it is not thewing_spanthatsolvenow reports. Switching it tocalculate_span(wings, wings[1].spanwise_direction)is one line, but it would change the rate derivatives of multi-wing bodies in a function that just landed, so it is not in here.Verification
test/solver/test_wing_directions.jlonmain'ssrc: 5 of 9 failed; the aspect-ratio testset alone: 0 of 2test/solver/test_wing_directions.jlgreen after, 14/14 (juliaserver, Julia 1.13)origin/mainat 46adf3d (Start ELLIPTIC circulation on each wing's own ellipse, so multi-wing bodies solve #366, elliptic circulation per wing), whosespanwise_extentconflicted with this branch'scalculate_span:test_wing_directions14,test_body_aerodynamics4909 (with Start ELLIPTIC circulation on each wing's own ellipse, so multi-wing bodies solve #366's two-wing elliptic tests),test_results30,test_solver50,test_wing_geometry302,test_settings39,test_stability27,test_forwarddiff10,test_unrefined_dist40, all passing (juliaserver, Julia 1.13). Earlier merges (BREAKING: name the public apparent wind va, va_vec and va_dist, with no deprecation aliases #368va_vecrename, Spread sliced sections evenly over the span, not over leading-edge arc length #350/stability_derivatives gives roll, pitch and yaw rate derivatives about solver.reference_point #356/Take the flap hinge heights from where the contour crosses the crease line #370) conflicted incalculate_resultsand the changelog and were checked the same wayPkg.test(), Julia 1.13) on d066ca6: PASS (5 min) · GitHub CI on d066ca6: PASS, all five jobs. On b3ce084 Julia 1.12 windows had failedtest/solver/test_forwarddiff.jl:100(LOOP, POLAR_MATRICES), the flake of test_forwarddiff.jl's POLAR_MATRICES check flakes at ~4%, and the oper #287 and Flaky POLAR_MATRICES forwarddiff test on Windows / Julia 1.12 (knot-proximity in piecewise-linear polar interpolation) #360 that also fails onmain's own push runsmain'ssrc(46adf3d) and this branch's, same script, inputs and axescalc_forces!stays zero-alloc, the per-panel work is unchanged, and the loop only gains an outer loop over the wingscl/cd/cs_distributionof a second wing whose span differs from the first's change sign or meaning. That is the fix, but a plot or script that read them in the first wing's frame will now show different numbers.Scope
+148 / −64 ignoring whitespace (+212 / −128 plain, since the two loops gain one indent level).
src/is +66 / −64, flat, while adding the per-wing loops and the multi-wing span; the rest is the new 74-line test, oneruntests.jlline, two docs entries and the changelog. Searched forwings[1],spanwise_directionand.spaninsrc/: the otherwings[1]read isstability_derivatives'b, above. For the test wing I searchedtest/forfunction .*wing(andadd_section!builders:inviscid_wing(test_body_aerodynamics.jl),flat_plate_wing(test_solver.jl) andbuild_flat_wing(test_billowing.jl) are file-local flat wings, none of them rotatable or carrying a polar table. The newrectangular_wingis a fourth; folding all four into one keyword-driven builder intest/test_data_utils.jlrewrites three test files this PR does not otherwise open, so it is acleanup:PR of its own.Closes #358 · task
VortexStepMethod.jl-358