Skip to content

BREAKING: name the public apparent wind va, va_vec and va_dist, with no deprecation aliases - #368

Merged
1-Bart-1 merged 4 commits into
mainfrom
agent/348-breaking-rename-the-public-apparent-wind
Sep 21, 2026
Merged

1-Bart-1 merged 4 commits into
mainfrom
agent/348-breaking-rename-the-public-apparent-wind

Conversation

@1-Bort-1

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

Copy link
Copy Markdown
Member

TL;DR

Every public apparent-wind name now follows the #147 convention: va is the speed [m/s], va_vec the 3-vector, and va_dist / va_vec_dist the per-panel values. #349 already did this for locals, tests and docs. This PR finishes the public side, so va no longer means a vector at the API and a speed inside.

Bart chose option A on #348: no depwarn aliases. None of the old names comes back with a different shape, so old code fails at once instead of silently getting a speed where it expected a vector:

  • body_aero.va and panel.va give a FieldError, held by the new testset "body_aero.va and panel.va are removed".
  • Old keywords give "unsupported keyword argument".
  • Old YAML keys give FieldError: type ConditionSettings has no field wind_speed, available fields: va, ....

What was renamed

Old New
body_aero.va (property), field _va body_aero.va_vec; the field is va_vec
va= on BodyAerodynamics(...) and reinit! va_vec=
Panel.va Panel.va_vec
VSMSolution._va_dist, va_unrefined_dist va_vec_dist, va_vec_unrefined_dist
solver.br.va_norm_dist va_dist
v_a_dist on BodyAerodynamics and solver.lr v_rel_dist
SemiInfiniteFilament.vel_mag va
v_a= on plot_polars / plot_combined_analysis va=
linearize(...; va_idxs) va_vec_idxs
calculate_results key "va_ref" "va_ref_vec"
YAML condition.wind_speed, airfoil.v_app va
  • v_a_dist holds |v_rel × y_airf|, not the apparent wind speed, so it takes the name its locals already had, v_rel_dist. Its field docstring now says what it holds.
  • "va_ref_vec" matches the local that fills it.
  • The repo's own YAML files are renamed with it: the 3 settings files under data/, 4 under test/, and the inline YAML in test_settings.jl.

The body_aero.va_vec property still throws after set_va! with a per-panel inflow, as body_aero.va did. Internal readers that relied on _va skipping that check (make_dual_shadow, linearize, the Makie extension) now read getfield(body_aero, :va_vec), including the dual-shadow type check in linearize.

This branch is stacked on #346, which adds new va_idxs= and condition.wind_speed callers. Its apparent_wind, stability_derivatives, trim_angle and coeffs_at_angles take the speed as va instead of wind_speed, and its changelog line follows.

Found on the way, not changed

  • airfoil.va (the speed the Reynolds number is taken at) and condition.va now share a name. The thread asked for va for both; say if you want the airfoil one as va_ref.
  • has_distributed_va keeps its name. It is a flag about the inflow, not a quantity with a shape. Say if you want has_va_vec_dist.
  • The spanwise sweep in plot_combined_analysis builds its inflow as [cos α cos β, sin β, sin α] * va. apparent_wind uses sin α cos β for z, so the two disagree when β ≠ 0. Using apparent_wind there would change the plotted numbers, so it is left for its own thread.
  • VSMSolution still has _chord_dist, _x_airf_dist, _y_airf_dist and _z_airf_dist with _ prefixes. Those are not apparent-wind names, so they are left out of this PR.

Follow-ups this needs

Verification

  • Reproduced first: n/a, this is a rename. The new removed-name testset was not run against the unrenamed code, where body_aero.va and panel.va return the vector.
  • Fresh juliaserver session, test env, Julia 1.13: runtests.jl filtered to bench, body_aerodynamics, filament, panel, settings, solver, yaml_geometry, plotting and verification. 5896 passed, 0 failed, 0 errored.
  • After the last wrap commit: body_aerodynamics and solver re-run, 5019 passed, 0 failed, 0 errored.
  • After the review fixes (c2f1fa3): test_body_aerodynamics.jl, examples env, Julia 1.13: 4905 passed, 0 failed, 0 errored. That includes the ForwardDiff linearize testset, which goes through the dual shadow.
  • Examples, examples env: linearize_check, pyramid_model, rectangular_wing, V3_kite, obj_to_yaml_kite, stall_model and ram_air_kite run without error. billowing got past its renamed lines 86 and 92, then stopped at line 109 on GLMakie's "can not display a scene in multiple" in the shared session. Its va= plot call is covered by test_plotting.jl's four va= calls.
  • Local CI mirror (agent ci-local, full Pkg.test(), Julia 1.13) on ae38bd2: PASS in 11 min, exit 0. It was not re-run on c2f1fa3, which only moves one test and one getfield. · GitHub CI on c2f1fa3: PASS (1.12 ubuntu, windows and macOS, 1.13 ubuntu, and Documentation).
  • Docs build: the Documentation CI job passed on c2f1fa3. It was not built locally, since docs/ has no manifest. No symbol is added to or removed from the exports.
  • Benchmark: n/a
  • Risk: a caller outside this repo that sets body_aero.va = ... or passes va= now errors, which is the intent. SymbolicAWEModels is the one known such caller.

Scope

+300 / -272 across 47 files, all rename. The only other changes are the field docstrings for va_vec and v_rel_dist, the rewrapped and realigned lines, the new removed-name testset, and the CHANGELOG entry. The net +28 lines come from wrapping lines the rename pushed past 92 columns, plus the changelog and the six-line testset. Stack: on #346, which follows #349 (merged).

Closes #348 · task VortexStepMethod.jl-348

1-Bort-1 and others added 3 commits September 21, 2026 13:59
body_aero.va -> va_vec (field _va -> va_vec), va= -> va_vec= on
BodyAerodynamics and reinit!, Panel.va -> va_vec, VSMSolution._va_dist ->
va_vec_dist, va_unrefined_dist -> va_vec_unrefined_dist, BaseResult.va_norm_dist
-> va_dist, v_a_dist -> v_rel_dist, SemiInfiniteFilament.vel_mag -> va,
v_a= -> va= on the plot functions, linearize va_idxs= -> va_vec_idxs=, the
"va_ref" result key -> "va_ref_vec", and the settings keys
condition.wind_speed and airfoil.v_app -> va.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 21, 2026

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

Copy link
Copy Markdown
Member 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 · 2 inline, 0 off the diff

Good

  • Rename is complete: rg over src/test/examples/docs finds no _va, v_a_dist, va_norm_dist, wind_speed, v_app, vel_mag, v_a=, va_idxs, "va_ref" or _va_dist outside CHANGELOG
  • No depwarn aliases, matching option A on #348: none of the old names returns a value of a different shape, so old callers fail loudly
  • getproperty gets simpler: nested ifs become one guard with an early throw, and the error message names the new fields
  • Unguarded internal readers (set_va!, setproperty!, make_dual_shadow, the Makie ext) use getfield(..., :va_vec) as the card says, so a distributed inflow does not trip the guard
  • v_rel_dist is the right name: calc_forces! already bound it to v_rel_dist, and it holds |v_rel × y_airf|, not a speed
  • The reinit! docstring signature stops listing refine_mesh/recompute_mapping/sort_sections, which the method does not accept
  • CHANGELOG BREAKING entry lists every rename and matches the card's table

Not good

  • src/solver.jl:1353 — This is the one internal reader still going through the guarded .va_vec property; the card says they all use getfield. Using getfield(shadow[1], :va_vec) keeps one rule and cannot throw if the shadow ever gets distributed inflow.
  • test/body_aerodynamics/test_body_aerodynamics.jl:498 — The removed-name check is added to an unrelated omega testset. Move it to its own @testset "body_aero.va is removed" or into the distributed-inflow access testset so the test name says what it guards.
  • linearize line 1353 still reads shadow[1].va_vec through the guarded property, unlike the other internal readers the card lists. It is safe only because the shadow is always set uniform
  • @test_throws FieldError body_aero.va sits in the "set_va! with omega on multi-wing body" testset, which is about something else, so the test name does not say what it protects
  • The card says the new FieldError test was not run against the unrenamed code, so it has not been shown to fail before the change
  • The YAML and docs/settings.md inline comments lost their column alignment after wind_speed:/v_app: became va:, and so did va_vec_idxs = in examples/linearize_check.jl
  • airfoil.va (the Reynolds reference speed) now has the same name as condition.va (the flight condition) with a different meaning. A va_ref-style name would keep them apart
  • The local CI mirror and GitHub CI are still unchecked in the card

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/solver.jl Outdated
else
shadow = shadow_ref[]
if shadow === nothing || eltype(shadow[1]._va) !== TI
if shadow === nothing || eltype(shadow[1].va_vec) !== TI

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

MINOR: This is the one internal reader still going through the guarded .va_vec property; the card says they all use getfield. Using getfield(shadow[1], :va_vec) keeps one rule and cannot throw if the shadow ever gets distributed inflow.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in c2f1fa3: the shadow check reads getfield(shadow[1], :va_vec).

@test !body_aero.has_distributed_va
@test body_aero.va ≈ va_vec
@test body_aero.va_vec ≈ va_vec
@test_throws FieldError body_aero.va

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

MINOR: The removed-name check is added to an unrelated omega testset. Move it to its own @testset "body_aero.va is removed" or into the distributed-inflow access testset so the test name says what it guards.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in c2f1fa3: the check has its own @testset "body_aero.va and panel.va are removed".

@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 21, 2026
…its own testset

Also realign the inline comments after the va: and va_vec_idxs renames.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.21429% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/panel.jl 66.66% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@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 labels Sep 21, 2026
@1-Bort-1
1-Bort-1 requested a review from 1-Bart-1 September 21, 2026 13:26
Base automatically changed from agent/330-add-rigid-body-stability-derivatives-and to main September 21, 2026 13:53
@1-Bart-1
1-Bart-1 merged commit 8754b6d into main Sep 21, 2026
6 checks passed
@1-Bart-1
1-Bart-1 deleted the agent/348-breaking-rename-the-public-apparent-wind branch September 21, 2026 13:54
@1-Bort-1 1-Bort-1 added agent:done Agent task state and removed agent:review Agent task state labels Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:done Agent task state

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BREAKING: rename the public apparent-wind API to va / va_vec / va_dist

2 participants