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 (7 min, Julia 1.12.7, one cell of the matrix) |
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 and then callsset_va!with the original body'sreference_point. Without that, 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
The name.
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.The per-panel method is left alone. The issue suggests giving
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.A possible docstring error, not changed. The
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.Overlap with linearize and make_dual_shadow take a BodyAerodynamics with several wings #355. linearize and make_dual_shadow take a BodyAerodynamics with several wings #355 (several wings in
linearize) also rewrites howmake_dual_shadowbuilds the shadow body, and it targetsmain. Whichever of the two merges second has to keep passingreference_pointinto the shadow. The ForwardDiff pivot testset here fails if it does not.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 83867a3 (juliaserver, fresh session;test_forwarddiff.jlagain at 8a2b61d):test/body_aerodynamics/test_body_aerodynamics.jl,test/body_aerodynamics/test_results.jl(linearize Jacobian 30/30),test/solver/test_viscous_drag_correction.jl(11 + 79),test/solver/test_solver.jl,test/solver/test_flow_curvature.jl,test/solver/test_moment_units.jl,test/solver/test_unrefined_dist.jl,test/solver/test_forwarddiff.jl(10/10).test/plotting/test_plotting.jl: not run locally; it is the only test of the Makie restore path.Julia 1.12 - windowson one assertion,AutoForwardDiff matches AutoFiniteDiff (LOOP, POLAR_MATRICES), at0.046430340012943265 < 1e-4, the same bits both times. The test is at fault, not this change; it is test_forwarddiff.jl's POLAR_MATRICES check flakes at ~4%, and the oper #287. Whether that check passes depends on the runner, not on the commit. A manual CI run on the same commit passes on Windows with the bitsmaingets there (norm_fwd2.618632581116621,norm_fd2.6186324991070755). A manual run onmainlands on the other polar table in 1.13 ubuntu and on macOS. Locally, on one generated table, this branch andmaingive identical Jacobians to the last bit. The numbers are on #287. The branch is up to date withorigin/main.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 are resolved to itsva_vec/va_vec_distnames, and the new tests use them too. Git mergedtest_forwarddiff.jlwithout a conflict but left a stalevathere, which would have failed at load; that is fixed in the merge commit. f8761aa (the POLAR_MATRICES grid change) merged cleanly.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 / −40 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