set_va! turns the body about a stored reference_point, and set_va!(body_aero, settings) applies yaw_rate - #353
Conversation
set_va!(body_aero, va, omega; reference_point) gives each panel va - omega × (control_point - reference_point). The point is stored on BodyAerodynamics so reinit!, the omega setter and the ForwardDiff shadow in linearize keep it; it starts at the origin. set_va!(body_aero, settings) passes condition.yaw_rate as omega about body z instead of dropping it. 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 · 1 inline, 0 off the diff
Good
- The per-panel formula
va - omega × (control_point - reference_point)inset_va!matches the card and the Python reference; the zero branch it drops was the same maths withomega = 0, so results with no turn rate do not change. - Storing the point on the body keeps it through every other caller:
reinit!(body_aerodynamics.jl:327),linearize(solver.jl:1320) and the Makie restore (MakieExt:1258) all callset_va!(ba, va, omega), so none of them resets it. make_dual_shadowcopiesreference_pointinto the shadow body, and the ForwardDiff testset now runs about a pivot with non-zero omega; the card shows this case failing before the fix (0.1395).- The new testset checks the keyword, the
omegasetter,reinit!and thereference_pointsetter, each against one helper, on a two-wing body. - The settings path gets
yaw_ratefrom °/s to rad/s about z, and the extended test asserts bothpanel.vaandbody_aero.omega; every shipped YAML hasyaw_rate: 0.0(grep), so no shipped setup changes. - The diff stays on what the card says: 7 files, +85/−37, the cleanup is named, and the doubts about the frame and the per-panel method are written down in the card, not buried in code.
Not good
src/body_aerodynamics.jl:39—BodyAerodynamics.reference_point(the point the body turns about) andSolver.reference_point(the moment reference) now share a name but mean different points, so a user who sets one will expect the other to follow. A field name is API once released, so choose now (rotation_center, as the card suggests, or say the two are meant to be the same point).- The
body_aero.reference_point = psetter callsset_va!(obj, obj._va, obj.omega), so on a body set up with the per-panel matrix method it silently replaces the distribution with its mean and zeroespitch_rate_dist. Theomegasetter already had this trap; this adds a second way in. - The settings docstring now says
yaw_rateis applied about Z_b, right next to aZ_b (down)label the card itself doubts. The sign users will expect foryaw_ratedepends on that label, so check the frame before release rather than after. docs/src/settings.mdnow describesyaw_rateas a turn rate about body z, but the field comment atsrc/settings.jl:15still says onlyyaw rate [°/s], so the two descriptions differ.- The ForwardDiff testset header puts
forat the end of the line and the loop variables on the next one, which is harder to scan than bindingturnsinline or splitting into two named testsets. test/plotting/test_plotting.jl, the only test of the Makie restore path, was not run, and neither CI result has been read yet.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| _va::MVector{3, T} = zeros(MVector{3, T}) | ||
| has_distributed_va::Bool = false | ||
| omega::MVector{3, T} = zeros(MVector{3, T}) | ||
| reference_point::MVector{3, T} = zeros(MVector{3, T}) |
There was a problem hiding this comment.
MINOR: BodyAerodynamics.reference_point (the point the body turns about) and Solver.reference_point (the moment reference) now share a name but mean different points, so a user who sets one will expect the other to follow. A field name is API once released, so choose now (rotation_center, as the card suggests, or say the two are meant to be the same point).
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…igin Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1-Bart-1
left a comment
There was a problem hiding this comment.
Rename to rotation_center. Yaw_rate is about z up, KA frame (KiteUtils.jl)
|
Local full suite: PASS (4 min, Julia 1.13.0, one cell of the matrix) |
…eference point Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-and set_va!(body_aero, settings) keeps both sides: the inflow comes from apparent_wind, and the body turns at settings.condition.yaw_rate about body_aero.reference_point, as #353 made it. Both Added entries in the changelog are kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TL;DR
set_va!(body_aero, va_vec, omega; reference_point)now gives each panelva_vec - omega × (control_point - reference_point), where it used to pivot about the origin only, andset_va!(body_aero, settings)passescondition.yaw_ratethrough asomegaabout body z. Before this, the only way to move the pivot was to shift the whole mesh withkite_body_origin, and a non-zeroyaw_ratein a settings file did nothing.What changed
reference_pointis a new field onBodyAerodynamics, zero by default, so every existing result stays the same. Theset_va!keyword defaults to that stored value and writes to it. The formula matches the Python package (awegroup/Vortex-Step-Method@b5cd5f1,BodyAerodynamics.py:522).set_va!is called again byreinit!, by thebody_aero.omega = ...setter, insidelinearizeand when the Makie helper restores the old state. Each of those would quietly go back to the origin. Storing the point, asomegaalready is, keeps it through all of them without touching those callers.body_aero.reference_point = pgoes throughset_va!the same way theomegasetter does.linearize.make_dual_shadowbuilds the shadow body from every wing, as linearize and make_dual_shadow take a BodyAerodynamics with several wings #355 made it, and then callsset_va!with the original body'sreference_point. The constructor takes noreference_point, sovaandomegago through that oneset_va!call instead of constructor keywords. Without it, the ForwardDiff Jacobian about a pivot is off by 14% from finite differences (below).yaw_rate. Converted from °/s and applied asomega = [0, 0, yaw_rate], as the Pythonva_initializedoes (its defaultbody_axisis +z). Every shippedvsm_settings*.yamlhasyaw_rate: 0.0, so no shipped setup changes.set_va!. The zero-omegabranch and the per-wing loop are gone. The loop walked the panels in the same order as a singleenumerate(panels), and withomega = 0the cross product is zero, so behaviour is the same (existing multi-wingomegatest still green).Where I would push back
reference_pointfollows the issue and the Python keyword, but in this packageSolver.reference_pointis the moment reference.body_aero.reference_pointandsolver.reference_pointare now two different points with the same name.rotation_centerwould be clearer; renaming is one sed if you prefer it.set_va!(body_aero, va_distribution::AbstractMatrix; pitch_rate_dist)the same keyword. That method takes noomega, so there is nothing to turn about. Python accepts an(n, 3)vatogether withbody_rates; if that is wanted, it is a separate change.set_va!(body_aero, settings)docstring labels X_b "forward" and Z_b "down". Positive alpha givesva_z = +sin α, and the test wings run from LE at x = 0 to TE at x = 1, which reads as x aft and z up. If so, the sign a user expects foryaw_rateis about z up. I did not verify the frame beyond that, so I left the labels as they are.Verification
set_va! with VSMSettings applies the yaw rate about body z: 1 pass, 5 fail.set_va! rotates the body about reference_point:MethodError(no keyword).set_va!fixed but not yetmake_dual_shadow:ForwardDiff matches FiniteDiff about [0.5, 4.0, 0.0]evaluated0.1395 < 0.001.origin/mainat 086ee32 (juliaserver, fresh session, Julia 1.13.0):test/body_aerodynamics/test_body_aerodynamics.jl(4903),test/body_aerodynamics/test_results.jl(linearize Jacobian 30/30),test/filament/test_bound_filament.jl(225),test/solver/test_forwarddiff.jl(10/10, pivot case included),test/solver/test_moment_units.jl,test/solver/test_viscous_drag_correction.jl,test/solver/test_solver.jl,test/solver/test_unrefined_dist.jl,test/solver/test_flow_curvature.jl,test/settings/test_settings.jl.test/plotting/test_plotting.jl: not run locally; it is the only test of the Makie restore path.POLAR_MATRICEScheck included. The branch is up to date withorigin/mainat 086ee32. On the previous head 8a2b61d both passed; GitHub needed three attempts becauseJulia 1.12 - windowsfailedAutoForwardDiff matches AutoFiniteDiff (LOOP, POLAR_MATRICES)twice at0.046430340012943265 < 1e-4. That is test_forwarddiff.jl's POLAR_MATRICES check flakes at ~4%, and the oper #287: which polar table a runner builds decides the check, and a manual run onmainsplits the same way (numbers on #287). Locally this branch andmaingive identical Jacobians to the last bit.main. Add the Gaunaa et al. 2024 spanwise-flow viscous drag correction as an opt-in solver setting #352 (viscous drag correction) conflicted only inCHANGELOG.md; both entries are kept. The correction reads the per-panel inflow thatset_va!has already set, so it sees the pivot. Name the apparent wind va, va_vec and va_dist everywhere except the public API #349 (theva/va_vecrename) conflicted inset_va!and in the settings testset; both use its names. linearize and make_dual_shadow take a BodyAerodynamics with several wings #355 (several wings inlinearize) conflicted inmake_dual_shadow, resolved as above. Bound and trailing vortices share one segment kernel, so the trailing core induces azimuthal velocity #351 (trailing-vortex core) conflicted only inCHANGELOG.md; both entries are kept.set_va!). REUSE: not used here.set_va!runs once per operating point, not inside the solve loop.set_va!pivoting about the origin after an earlier call set a non-zeroreference_point. The stored point now carries over by design.Scope
+81 / −37 across 7 files against
main.src/body_aerodynamics.jlgets the field, the setter,set_va!(net shorter after dropping the branch) and the settings method.src/solver.jlgets the shadow construction. Tests: one new testset, the settings testset extended to a non-zeroyaw_rate(create_temp_wing_settingsgained ayaw_ratekeyword), and the ForwardDiff INVISCID testset run about the origin and about a pivot. Also CHANGELOG and one YAML comment indocs/src/settings.md.Closes #329 · task
VortexStepMethod.jl-329