Skip to content

set_va! turns the body about a stored reference_point, and set_va!(body_aero, settings) applies yaw_rate - #353

Open
1-Bort-1 wants to merge 4 commits into
mainfrom
agent/329-set-va-rotates-the-body-about-the-origin
Open

1-Bort-1 wants to merge 4 commits into
mainfrom
agent/329-set-va-rotates-the-body-about-the-origin

Conversation

@1-Bort-1

@1-Bort-1 1-Bort-1 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

TL;DR

set_va!(body_aero, va_vec, omega; reference_point) now gives each panel va_vec - omega × (control_point - reference_point), where it used to pivot about the origin only, and set_va!(body_aero, settings) passes condition.yaw_rate through as omega about body z. Before this, the only way to move the pivot was to shift the whole mesh with kite_body_origin, and a non-zero yaw_rate in a settings file did nothing.

What changed

  • Pivot. reference_point is a new field on BodyAerodynamics, zero by default, so every existing result stays the same. The set_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).
  • Why it is stored. A keyword that is not stored gets lost: set_va! is called again by reinit!, by the body_aero.omega = ... setter, inside linearize and when the Makie helper restores the old state. Each of those would quietly go back to the origin. Storing the point, as omega already is, keeps it through all of them without touching those callers. body_aero.reference_point = p goes through set_va! the same way the omega setter does.
  • linearize. make_dual_shadow builds the shadow body and then calls set_va! with the original body's reference_point. Without that, the ForwardDiff Jacobian about a pivot is off by 14% from finite differences (below).
  • yaw_rate. Converted from °/s and applied as omega = [0, 0, yaw_rate], as the Python va_initialize does (its default body_axis is +z). Every shipped vsm_settings*.yaml has yaw_rate: 0.0, so no shipped setup changes.
  • Cleanup in set_va!. The zero-omega branch and the per-wing loop are gone. The loop walked the panels in the same order as a single enumerate(panels), and with omega = 0 the cross product is zero, so behaviour is the same (existing multi-wing omega test still green).

Where I would push back

  • The name. reference_point follows the issue and the Python keyword, but in this package Solver.reference_point is the moment reference. body_aero.reference_point and solver.reference_point are now two different points with the same name. rotation_center would 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 no omega, so there is nothing to turn about. Python accepts an (n, 3) va together with body_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 gives va_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 for yaw_rate is 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 how make_dual_shadow builds the shadow body, and it targets main. Whichever of the two merges second has to keep passing reference_point into the shadow. The ForwardDiff pivot testset here fails if it does not.

Verification

  • Red before, on unchanged code:
    • 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).
    • With set_va! fixed but not yet make_dual_shadow: ForwardDiff matches FiniteDiff about [0.5, 4.0, 0.0] evaluated 0.1395 < 0.001.
  • Green after merging origin/main at 83867a3 (juliaserver, fresh session; test_forwarddiff.jl again 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.
  • Local CI mirror on 8a2b61d: PASS, 7 min, Julia 1.12.7, one cell. GitHub CI on 8a2b61d: PASS, 5 of 5, on the third attempt. Attempts 1 and 2 failed Julia 1.12 - windows on one assertion, AutoForwardDiff matches AutoFiniteDiff (LOOP, POLAR_MATRICES), at 0.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 bits main gets there (norm_fwd 2.618632581116621, norm_fd 2.6186324991070755). A manual run on main lands on the other polar table in 1.13 ubuntu and on macOS. Locally, on one generated table, this branch and main give identical Jacobians to the last bit. The numbers are on #287. The branch is up to date with origin/main.
  • Three merges from main. Add the Gaunaa et al. 2024 spanwise-flow viscous drag correction as an opt-in solver setting #352 (viscous drag correction) conflicted only in CHANGELOG.md; both entries are kept. The correction reads the per-panel inflow that set_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 (the va / va_vec rename) conflicted in set_va! and in the settings testset. Both are resolved to its va_vec / va_vec_dist names, and the new tests use them too. Git merged test_forwarddiff.jl without a conflict but left a stale va there, which would have failed at load; that is fixed in the merge commit. f8761aa (the POLAR_MATRICES grid change) merged cleanly.
  • Docs: no new exported symbol (a field and a keyword on the documented set_va!). REUSE: not used here.
  • Benchmark: n/a, not measured. set_va! runs once per operating point, not inside the solve loop.
  • Risk: a caller that relies on set_va! pivoting about the origin after an earlier call set a non-zero reference_point. The stored point now carries over by design.

Scope

+81 / −40 across 7 files against main. src/body_aerodynamics.jl gets the field, the setter, set_va! (net shorter after dropping the branch) and the settings method. src/solver.jl gets the shadow construction. Tests: one new testset, the settings testset extended to a non-zero yaw_rate (create_temp_wing_settings gained a yaw_rate keyword), and the ForwardDiff INVISCID testset run about the origin and about a pivot. Also CHANGELOG and one YAML comment in docs/src/settings.md.

Closes #329 · task VortexStepMethod.jl-329

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 1-Bort-1 added agent:running Agent task state agent:ci Agent task state and removed agent:running Agent task state labels Sep 16, 2026

@1-Bort-1 1-Bort-1 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review (advisory)

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

Good

  • The per-panel formula va - omega × (control_point - reference_point) in set_va! matches the card and the Python reference; the zero branch it drops was the same maths with omega = 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 call set_va!(ba, va, omega), so none of them resets it.
  • make_dual_shadow copies reference_point into 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 omega setter, reinit! and the reference_point setter, each against one helper, on a two-wing body.
  • The settings path gets yaw_rate from °/s to rad/s about z, and the extended test asserts both panel.va and body_aero.omega; every shipped YAML has yaw_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:39BodyAerodynamics.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).
  • The body_aero.reference_point = p setter calls set_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 zeroes pitch_rate_dist. The omega setter already had this trap; this adds a second way in.
  • The settings docstring now says yaw_rate is applied about Z_b, right next to a Z_b (down) label the card itself doubts. The sign users will expect for yaw_rate depends on that label, so check the frame before release rather than after.
  • docs/src/settings.md now describes yaw_rate as a turn rate about body z, but the field comment at src/settings.jl:15 still says only yaw rate [°/s], so the two descriptions differ.
  • The ForwardDiff testset header puts for at the end of the line and the loop variables on the next one, which is harder to scan than binding turns inline 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.

Comment thread src/body_aerodynamics.jl
_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})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

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 1-Bart-1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to rotation_center. Yaw_rate is about z up, KA frame (KiteUtils.jl)

@1-Bort-1

1-Bort-1 commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Local full suite: PASS (7 min, Julia 1.12.7, one cell of the matrix)

@1-Bort-1 1-Bort-1 added agent:queued Agent task state agent:running Agent task state and removed agent:running Agent task state agent:queued Agent task state labels Sep 17, 2026
…ange

#349 renamed the apparent-wind locals and arguments; set_va!'s pivot, the
yaw_rate settings method and the new tests take its va_vec / va_vec_dist
names. CHANGELOG: #328's second Fixed heading folded into the one above.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@1-Bort-1 1-Bort-1 added agent:queued Agent task state agent:running Agent task state agent:ci Agent task state and removed agent:running Agent task state agent:queued Agent task state agent:ci Agent task state labels Sep 17, 2026
@1-Bort-1 1-Bort-1 added agent:running Agent task state agent:ci Agent task state and removed agent:queued Agent task state agent:running Agent task state labels Sep 17, 2026
@1-Bort-1 1-Bort-1 added agent:queued Agent task state agent:running Agent task state and removed agent:ci Agent task state agent:queued Agent task state labels Sep 17, 2026
@1-Bort-1 1-Bort-1 added agent:queued Agent task state agent:running Agent task state agent:ci Agent task state agent:review Agent task state and removed agent:running Agent task state agent:queued Agent task state agent:ci Agent task state agent:review Agent task state labels Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:running Agent task state

Projects

None yet

Development

Successfully merging this pull request may close these issues.

set_va! rotates the body about the origin only, and condition.yaw_rate is read but never applied

2 participants