Start ELLIPTIC circulation on each wing's own ellipse, so multi-wing bodies solve - #366
Merged
1-Bart-1 merged 6 commits intoSep 21, 2026
Merged
Conversation
calculate_circulation_distribution_elliptical_wing loops over the wings with a running panel offset and measures each control point along its wing's spanwise_direction from that wing's mid-span, so a body with more than one wing no longer throws. spanwise_extent gives the (lo, hi) projection that calculate_span and the ellipse both take; the scratch field BodyAerodynamics.y it used to fill goes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1-Bort-1
commented
Sep 21, 2026
1-Bort-1
left a comment
Member
Author
There was a problem hiding this comment.
Independent review (advisory)
Verdict: APPROVE WITH COMMENTS · 1 inline, 0 off the diff
Good
- Matches the card's plan: the loop with a running panel offset replaces the single-wing guard, which is what #357 proposed; #358 is left out on purpose and named
- One source for the span:
calculate_spanis nowhi - loofspanwise_extent, so the span and the mid-span can't drift apart (wing_geometry.jl:1693-1707) - The per-wing offset loop follows the idiom already used at body_aerodynamics.jl:119/319 and solver.jl:532; no new offset helper was invented (checked with rg)
- Removing
BodyAerodynamics.yis safe inside the repo: rg over src/ext/test/examples/docs finds no reader ofbody_aero.y - Tests cover what the card claims: the fin along z at 5 m and the wing shifted 5 m in y check the per-wing axis and mid-span, and ELLIPTIC and ZEROS converging to the same gamma shows the converged result doesn't depend on the start
- The
@debuglines and the stale 'Returns: nothing' docstring go away; the new docstrings are short and state what the code does
Not good
CHANGELOG.md:38— The changelog doesn't mention that the documentedBodyAerodynamicsfieldywas removed. This CHANGELOG marks removed public surface asBREAKING:, so code that passedy=to the keyword constructor breaks with nothing written down. Add aBREAKING:line, or keep the field until Bart decides as the card asks.normalize(wing.spanwise_direction)is computed both inspanwise_extentand in the caller at body_aerodynamics.jl:508; havingspanwise_extentalso return the axis, or take it, would leave one copy- No test covers a lone wing off y = 0, the behaviour change the card calls out; it is only covered indirectly as the second wing of the pair
- The docs build and
ci-localare unverified;spanwise_extentis listed in private_functions.md, but only CI will confirm thatmake.jlis happy
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…AKING Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… the two-wing test Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
Local full suite: PASS (6 min, Julia 1.13.0, one cell of the matrix) |
…itial-circulation-for-a-body- # Conflicts: # CHANGELOG.md
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1-Bart-1
reviewed
Sep 21, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1-Bart-1
approved these changes
Sep 21, 2026
1-Bart-1
deleted the
agent/357-elliptic-initial-circulation-for-a-body-
branch
September 21, 2026 20:46
1-Bort-1
added a commit
that referenced
this pull request
Sep 21, 2026
#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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
solve!withtype_initial_gamma_distribution = ELLIPTICnow works on aBodyAerodynamicswith more than one wing, where it threwArgumentError("Multiple wings not yet implemented"). Each wing gets an ellipse over its own span, along its ownspanwise_direction, centred on its own mid-span; that also unblockslinearizeon a multi-wing body with that setting.What changed
calculate_circulation_distribution_elliptical_wingloops over the wings with a running panel offset, as the issue proposed. A control point's position is its projection on the wing's normalisedspanwise_directionminus the mid-point of the wing's span, and the ellipse is taken over that wing's span. On one wing centred on y = 0 with span along y, that is the same formula as before.The span's lower and upper ends come from a new private
spanwise_extent(wing). It is the projectioncalculate_spanalready did, now allocation-free, andcalculate_spanishi - loof it. So the span and the mid-span come from one place.Where I'd push back
ZEROS, and a new test pins the ellipse.BodyAerodynamics.yis removed. It was a private scratch buffer that only this function filled, so it gets no changelog entry.calc_forces!andcalculate_resultsusewings[1].spanwise_directionfor every wing) is the same class of bug in other functions. It is left to that issue.Searched for
span,spanwise_direction,extremaandpanel_offsetbefore addingspanwise_extent.unrefined_section_rangeis the only offset helper, and it indexes sections, not panels.Verification
ArgumentError: Multiple wings not yet implemented.test/body_aerodynamics/test_body_aerodynamics.jl"solve! on a two-wing body": 27 pass + 2 errored before, 31/31 after (juliaserver). New tests:spanwise_direction = z, 5 m up). Each wing's slice equalssqrt(1 - (2y/span)^2)of the centred wing. A lone wing shifted 5 m in y gets that ellipse too (the old formula gives[0.0, 0.0, 0.0, 0.0, 0.0, 0.0]there).test/solver/test_solver.jl(single-wing ELLIPTIC),test/solver/test_forwarddiff.jl(ELLIPTIC throughlinearize),test/wing_geometry/test_wing_geometry.jl(calculate_span).spanwise_extentonprivate_functions.md.mainthree times, each conflict inCHANGELOG.mdonly, both sides kept each time: 1d399ad (theva→va_vecBREAKING entry; the new test'sva=keyword becameva_vec=), e122ede (the polar-precision Fixed entry from Write polar CSV coefficients at 16 significant digits instead of 4 decimals #369), 0cc70c0 (theget_lower_upperFixed entry from Take the flap hinge heights from where the contour crosses the crease line #370; Spread sliced sections evenly over the span, not over leading-edge arc length #350 and stability_derivatives gives roll, pitch and yaw rate derivatives about solver.reference_point #356 merged cleanly, and neither touches the ellipse orspanwise_extent). On 0cc70c0,test_body_aerodynamics.jlandtest/solver/test_stability.jlpass 4936/4936 (juliaserver).AutoForwardDiff matches AutoFiniteDiff (LOOP, POLAR_MATRICES): at0.046430340012943265on 1d399ad and at0.057102607132145794on 0cc70c0. Every other cell passed. That is test_forwarddiff.jl's POLAR_MATRICES check flakes at ~4%, and the oper #287.mainfailed on the same assertion with the same bits at 086ee32, and again at0.05570289722278327on its own push run at adff953. On 0cc70c0 the Windows cell'snorm_fd(2.0741507267422183) is the tablemain's passing macOS cell used at a059747, but itsnorm_fwdcame out 2.05758 against 2.07415. This diff cannot move that test. It runs theZEROSstart, and on e122ede itslinearizejacobians were bit-identical (==) with this branch'ssrcand withorigin/main's.agent ci-local: PASS on 1d399ad (7 min) and on 0cc70c0 (8 min), Julia 1.13 · GitHub CI on 83b5ef6: all 7 checks green, Windows 1.12 included.Scope
+72 / −37 across 5 files. The source change is +27 / −34: the loop replaces the single-wing guard and the global ellipse, and
spanwise_extentreplaces the body ofcalculate_span. The other lines are the two testsets (+34), the changelog lines and the docs entry.Closes #357 · task
VortexStepMethod.jl-357