BREAKING: name the public apparent wind va, va_vec and va_dist, with no deprecation aliases - #368
Conversation
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
left a comment
There was a problem hiding this comment.
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_distoutside 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
getpropertygets 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) usegetfield(..., :va_vec)as the card says, so a distributed inflow does not trip the guard v_rel_distis the right name:calc_forces!already bound it tov_rel_dist, and it holds |v_rel × y_airf|, not a speed- The
reinit!docstring signature stops listingrefine_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_vecproperty; the card says they all usegetfield. Usinggetfield(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.linearizeline 1353 still readsshadow[1].va_vecthrough 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.vasits 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:becameva:, and so didva_vec_idxs =in examples/linearize_check.jl airfoil.va(the Reynolds reference speed) now has the same name ascondition.va(the flight condition) with a different meaning. Ava_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.
| else | ||
| shadow = shadow_ref[] | ||
| if shadow === nothing || eltype(shadow[1]._va) !== TI | ||
| if shadow === nothing || eltype(shadow[1].va_vec) !== TI |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
…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 Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
TL;DR
Every public apparent-wind name now follows the #147 convention:
vais the speed [m/s],va_vecthe 3-vector, andva_dist/va_vec_distthe per-panel values. #349 already did this for locals, tests and docs. This PR finishes the public side, sovano 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.vaandpanel.vagive aFieldError, held by the new testset "body_aero.va and panel.va are removed".FieldError: type ConditionSettings has no field wind_speed, available fields: va, ....What was renamed
body_aero.va(property), field_vabody_aero.va_vec; the field isva_vecva=onBodyAerodynamics(...)andreinit!va_vec=Panel.vaPanel.va_vecVSMSolution._va_dist,va_unrefined_distva_vec_dist,va_vec_unrefined_distsolver.br.va_norm_distva_distv_a_distonBodyAerodynamicsandsolver.lrv_rel_distSemiInfiniteFilament.vel_magvav_a=onplot_polars/plot_combined_analysisva=linearize(...; va_idxs)va_vec_idxscalculate_resultskey"va_ref""va_ref_vec"condition.wind_speed,airfoil.v_appvav_a_distholds|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.data/, 4 undertest/, and the inline YAML intest_settings.jl.The
body_aero.va_vecproperty still throws afterset_va!with a per-panel inflow, asbody_aero.vadid. Internal readers that relied on_vaskipping that check (make_dual_shadow,linearize, the Makie extension) now readgetfield(body_aero, :va_vec), including the dual-shadow type check inlinearize.This branch is stacked on #346, which adds new
va_idxs=andcondition.wind_speedcallers. Itsapparent_wind,stability_derivatives,trim_angleandcoeffs_at_anglestake the speed asvainstead ofwind_speed, and its changelog line follows.Found on the way, not changed
airfoil.va(the speed the Reynolds number is taken at) andcondition.vanow share a name. The thread asked forvafor both; say if you want the airfoil one asva_ref.has_distributed_vakeeps its name. It is a flag about the inflow, not a quantity with a shape. Say if you wanthas_va_vec_dist.plot_combined_analysisbuilds its inflow as[cos α cos β, sin β, sin α] * va.apparent_windusessin α cos βfor z, so the two disagree when β ≠ 0. Usingapparent_windthere would change the plotted numbers, so it is left for its own thread.VSMSolutionstill has_chord_dist,_x_airf_dist,_y_airf_distand_z_airf_distwith_prefixes. Those are not apparent-wind names, so they are left out of this PR.Follow-ups this needs
._va,panel.vaandsolver.lr.v_a_dist. After this is released it needsgetfield(body_aero, :va_vec)(or.va_vec),panel.va_vecandsolver.lr.v_rel_dist.va_idxs=. Whichever of stability_derivatives gives roll, pitch and yaw rate derivatives about solver.reference_point #356 and this PR merges second takes the other's names.Verification
body_aero.vaandpanel.vareturn the vector.runtests.jlfiltered to bench, body_aerodynamics, filament, panel, settings, solver, yaml_geometry, plotting and verification. 5896 passed, 0 failed, 0 errored.test_body_aerodynamics.jl, examples env, Julia 1.13: 4905 passed, 0 failed, 0 errored. That includes the ForwardDifflinearizetestset, which goes through the dual shadow.linearize_check,pyramid_model,rectangular_wing,V3_kite,obj_to_yaml_kite,stall_modelandram_air_kiterun without error.billowinggot 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. Itsva=plot call is covered bytest_plotting.jl's fourva=calls.agent ci-local, fullPkg.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 onegetfield. · GitHub CI on c2f1fa3: PASS (1.12 ubuntu, windows and macOS, 1.13 ubuntu, and Documentation).docs/has no manifest. No symbol is added to or removed from the exports.body_aero.va = ...or passesva=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_vecandv_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