Give the two stale examples the point types that replaced WING - #328
Give the two stale examples the point types that replaced WING#3281-Bort-1 wants to merge 1 commit into
Conversation
`WING` was a role tag, not a dynamics type, and v0.13.0 replaced it with the point's real type plus station membership: `BODY_STATIC` riding the wing body on a rigid wing, `DYNAMIC` on a particle wing. Both examples still passed the bare constant, so `sam_tutorial.jl` died with `UndefVarError: WING` on the step that adds the kite and took `Setup Test` on `main` with it. Which type each example wants is decided by the wing it already builds. `kps4_comparison.jl` declares `PlateWing(...; dynamics_type=PARTICLE_DYNAMICS)` and its three points are already named by its stations, so they are `DYNAMIC` and nothing else changes. `sam_tutorial.jl` reaches the pre-created-objects `VSMWing`, which never forwards `dynamics_type` and is therefore always `RIGID_DYNAMICS`, so its six points are `BODY_STATIC` riding that wing; it also declares one `Station` per LE/TE pair, which a section-coupled rigid wing now requires since `auto_create_twist_surfaces!` was removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011XNpwmH8PE6Z4T1vWAVSht
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
- kps4's three points are correctly
DYNAMIC: its wing isPlateWing(...; dynamics_type=PARTICLE_DYNAMICS)and its existing 1-pointSTATICstations at examples/kps4_comparison.jl:174-181 already carry the membership, so nothing else had to move. BODY_STATICfor the tutorial's rigid VSM wing matches the type's own docs (src/system_structure/types.jl:35 and :67: rigid wing → BODY_STATIC riding the body); the choice is derivable, not a judgement call, as the card argues.- Station shape
(name, [le, te], DYNAMIC, 0.25)and LE-before-TE order copy the repo's own rigid wing, data/2plate_kite/rigid_structural_geometry.yaml:105-107, rather than inventing a second convention. - 3 stations against 3
wing_sectionsin data/2plate_kite/aero_geometry.yaml, LE x=-0.5 / TE x=+0.5 matching the pushed positions, socompute_spatial_station_mapping!'s n_stations ≤ n_unrefined holds 1:1. eachindex(stations)reaches the VSMWing 5th positionalstationsarg (src/system_structure/wing.jl:464) and resolves viato_name_refon Int names 1..3 — the refs and the Station names agree.- The
wing_zhoist is genuine tidying of touched code: one spelling ofset.l_tether + 6where there were two, and it is named in the PR description. - Scope matches the card:
git diff HEAD~1is exactly 3 files, +23/-11, no plan/scratch files tracked, no version bump, no dependency added. - The card is honest about what was not proven — kps4's KiteModels half unrunnable,
Setup Testnot triggered on PRs,reuse_lintnot run — rather than claiming green everywhere.
Not good
examples/sam_tutorial.jl:121— These stations areDYNAMICwith defaultstiffness=0.0, but no segment attaches to points n+1/n+2, so the twist DOF has no bridle couple to oppose an aero moment — the exact conditionvalidate_station_modessaysDYNAMICexists for, and the ≥2-point check passes only on the count. The tutorial builds but never solves, so it costs nothing today; it teaches readers a station shape that would drift if it were ever integrated, whereSTATIC(prescribed section twist on a rigid wing) is the coherent choice for a wing with no bridles.- The tutorial's station points carry no segments, so a
DYNAMICtwist DOF is declared wherevalidate_station_modes' stated reason forDYNAMIC(a bridle couple) does not exist; see finding. - The comment on examples/sam_tutorial.jl:112 sits above
wing_z = ..., which it does not describe, and mostly restates the loop below it — the rubric's default is zero. - Both finds (kps4 can't run without a
KiteModelsdep; setup-test has nopull_requesttrigger) are documented only in the PR body; CLEAN_CODE §5 wants a genuinely different concern filed as a linked issue. - The CHANGELOG entry runs six narrative lines including the v0.13.0 back-story; two or three would carry the same user-visible fact.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| push!(points, Point(n + 2, [0.5, y, wing_z], | ||
| WING; wing=1, transform=1, extra_mass=0.1)) | ||
| BODY_STATIC; wing=1, transform=1, extra_mass=0.1)) | ||
| push!(stations, Station(i, [n + 1, n + 2], DYNAMIC, 0.25)) |
There was a problem hiding this comment.
MINOR: These stations are DYNAMIC with default stiffness=0.0, but no segment attaches to points n+1/n+2, so the twist DOF has no bridle couple to oppose an aero moment — the exact condition validate_station_modes says DYNAMIC exists for, and the ≥2-point check passes only on the count. The tutorial builds but never solves, so it costs nothing today; it teaches readers a station shape that would drift if it were ever integrated, where STATIC (prescribed section twist on a rigid wing) is the coherent choice for a wing with no bridles.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Local full suite: FAIL (28 min, Julia 1.12.7, one cell of the matrix) |
TL;DR
sam_tutorial.jlandkps4_comparison.jlstill passed theWINGDynamicsTypethat v0.13.0 removed, so the tutorial died withUndefVarError: WINGon the step that adds the kite and has been takingSetup Testdown onmainsince 2026-09-07. Each example now carries the point type its own wing implies —DYNAMICfor the particle wing,BODY_STATICfor the rigid one — and the tutorial declares the stations a section-coupled rigid wing has needed sinceauto_create_twist_surfaces!went away.Which type is not a judgement call
#324 left the choice open, on the grounds that
BODY_STATICversusDYNAMICis a call about what a tutorial is for. It is not: the removal commit8516368bsays "points now carry real types — DYNAMIC (particle wing) or BODY_STATIC riding the wing body (rigid wing)", and thePointandDynamicsTypedocstrings say the same. The type follows the wing the example already builds, so each file has exactly one right answer and they are different answers.kps4_comparison.jlbuildsPlateWing(...; dynamics_type=PARTICLE_DYNAMICS)and its three points are already the sole members of its three stations, so they areDYNAMICand the rest of the file is untouched — three words.sam_tutorial.jlcallsSymbolicAWEModels.Wing(1, vsm_aero, vsm_wing, vsm_solver, ...), which dispatches to the pre-created-objectsVSMWing. That constructor never forwardsdynamics_type, so the wing is alwaysRIGID_DYNAMICSwithAeroLinearized, and its six points areBODY_STATICriding it (wing=1: a wing is a body). That wing is also section-coupled and declared no stations, which is its own error now thatauto_create_twist_surfaces!is gone — "Section-coupled aero on RIGID wing 1 requires explicit stations covering its LE/TE structural sections; none were declared". So step 4 builds oneStationper LE/TE pair in the loop that already builds the pair, and hands them to the wing.validate_station_modeswantsDYNAMICtwist to sit on a rigid wing and to have at least two points a station, which is what an LE/TE pair is, andmoment_frac0.25 matchesdata/2plate_kite/rigid_structural_geometry.yaml, the repo's own rigid wing.The loop's
wing_zmoved three lines up so the wing and the points read one definition of it rather than two spellings ofset.l_tether + 6.What I found and did not fix
examples/kps4_comparison.jlcannot run in any environment this repository ships. Its line 18 isusing KiteModels, andKiteModelsappears in none of this repo's fiveProject.tomlfiles. That is why it is absent from theSetup Testrun list, and why its copy of this bug went unannounced. I could only exercise the half I changed; see Verification. Adding the dependency is a decision about whether that comparison is meant to be runnable, which is not mine to take inside this diff..github/workflows/setup-test.ymltriggers onpushtomainandworkflow_dispatchonly. Nothing runs the examples on a pull request, so an example can break and only announce it after it is merged — which is exactly what happened here, and it is why no check on this pull request runs an example at all; the local run of that job's own script, below, is what stands in for it. I would addpull_requestto the trigger, but at 120 minutes a run that is a real cost on every pull request, so it deserves its own thread rather than a line in this one.test/test_helpers.jl:50asserts that each trackedManifest-v1.1x.toml.defaultis no older thanProject.toml, as a "you changed the project, regenerate the manifests" reminder. The proxy is mtime, and the gate's own step 8 trips it:git merge origin/mainrewrites every working-tree file whose content moved since the base, andaaf26519"Release SymbolicAWEModels v0.17.0" changesProject.toml. The reflog reads9de13b34 HEAD@{2026-09-12 22:05:25}: merge origin/main: Fast-forwardandProject.toml's mtime is 22:05:25 to the second, while the.defaults still carrybin/install's 21:19:40 — so1.7892407808324673e9 >= 1.7892435240518901e9is false, for both manifests, forever in this worktree.All three files are byte-identical to
main(git diff origin/main -- Project.toml 'Manifest-*.default'is empty) and this branch touches none of them. CI cannot see it, because a fresh checkout stamps every file within the same second. #305 is open on exactly this and proposes the content check that replaces the mtime proxy; I left the test alone and put the mechanism, which the report there does not have, on that issue — regenerating the manifests to go green would be a lie in the diff, and rewriting the check is that issue's change, not this one's.Verification
LoadError: UndefVarError: WING not defined,examples/sam_tutorial.jl:119— the signature Two examples still use the removed WING DynamicsType, and Setup Test has been red on main since 2026-09-07 #324 quotes from CIexamples/sam_tutorial.jlred before, green after (juliaserver): "RIGID_DYNAMICS wing 1: COM=[[0.0, 0.0, 56.0]], I=[0.4, 0.15, 0.55]", then "Wing model created successfully"examples/kps4_comparison.jl: its own SymbolicAWEModels section, run verbatim without the KiteModels half, red before with the sameUndefVarErrorand green after — modelkps4_particle_plate_dynamic_11pnt_15seg_3grp_1wng_1wch_1bdybuilt and initialised,SymAWE section twists [deg]=[4.0, 10.0, 10.0]test/setup_integration.jl— the job that is red — run whole on this branch:Test Summary: End-user setup | Pass 19 | Total 19 | 64m07.4s, no errors. All nine examples in its run list and both README blocks pass; before,run sam_tutorial.jlwas the one error and 19 of 20 passed1303 passed, 1 failed, and the one failure is test_helpers' manifest-staleness check is an mtime comparison, so a dev'd dependency turns the local suite red #305's mtime check rather than this branch (above). The mirror runs underJULIA_TEST_FAILFAST=true, so it stopped attest_helpers.jland the 35 test files sorting after it ran on GitHub CI only. Nothing in the unit suite loadsexamples/Documentation, andreuse-lint, which is the licence gate./bin/reuse_lintcannot cover on the box for want ofreuse.test_helpers.jlpasses there, as test_helpers' manifest-staleness check is an mtime comparison, so a dev'd dependency turns the local suite red #305 predicts for a fresh checkout, and those cells are the only run that covers the 35 files the local mirror skipped.Setup Testdoes not run on pull requests, sotest/setup_integration.jlabove is what stands for itmain(9de13b34; v0.17.0 released mid-task, so the changelog entry opens a fresh## Unreleased)Scope
+23 / -11 across 3 files;
examples/kps4_comparison.jlis three occurrences ofWING→DYNAMICand nothing else. Closes #324.Closes #324 · task
SymbolicAWEModels.jl-324