Skip to content

Test that moments come out in N·m by scaling the wing's geometry - #335

Merged
1-Bart-1 merged 2 commits into
mainfrom
agent/209-add-tests-that-check-if-moment-have-righ
Sep 16, 2026
Merged

1-Bart-1 merged 2 commits into
mainfrom
agent/209-add-tests-that-check-if-moment-have-righ

Conversation

@1-Bort-1

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

Copy link
Copy Markdown
Member

TL;DR

A new test builds the same wing at two sizes and checks that every moment grows as k³, the per-span airfoil moment as k², and the moment coefficients not at all, on both solve! and solve. The v2.3.0 bug, a moment missing one chord factor, came through every existing moment test unnoticed because they all use chord = 1 or INVISCID sections, where cm = 0.

What the test does

If every length is multiplied by k while the inflow and polar stay the same, forces grow as k² and moments as k³. A moment that is really N (a chord factor short) grows as k² instead, and a coefficient divided by one length too few grows as k. So the test needs no reference numbers and no Unitful. It catches a missing or extra length factor in any moment output, not only the one term that was wrong in v2.3.0.

Setup: a rectangular POLAR_VECTORS wing, chord 1.5·k and span 6·k, with constant cm = −0.08, a reference point off the wing that scales with k, and k = 2. On this geometry the scaling holds exactly: 0.0 relative error in moment, moment_dist and moment_coeffs, so rtol = 1e-6 is loose. It covers force, moment, m_body_3D, moment_dist, moment_unrefined_dist, panel_moment_dist, moment_coeffs and moment_coeff_dist from solve!, and Mx/My/Mz, M_distribution and cmx/cmy/cmz from solve.

Fixed on the way

VSMSolution's docstring listed lift_dist, drag_dist and panel_moment_dist as [N] and [Nm]. They are per unit span (cl·q·c, cm·q·c²), so they are now documented as [N/m] and [Nm/m]. The code is unchanged, and the new test pins panel_moment_dist at k².

Found, not done

  • .github/workflows/CI.yml:22 has fail-fast: false. That belongs in a separate cleanup: PR, not this one.

Verification

  • Reproduced first: the old bug, put back by hand on the rebased code. With panel_moment changed from chord^2 to chord, 10 of 18 checks fail (7 in solve!, 3 in solve), e.g. solve Mz: -9104.95 ≈ -9815.41. With * c_ref dropped from the four coefficient divisions, 5 of 18 fail. Source restored after each run.
  • test/solver/test_moment_units.jl green on unchanged source, 17/17 after merging the two status checks (juliaserver, fresh session on the rebased branch)
  • Local CI mirror (agent ci-local, Julia 1.12.7): PASS in 8 min, exit 0
  • GitHub CI: PASS on 78bba14. The first attempt of Julia 1.12 - ubuntu-latest failed one check outside this change: test_forwarddiff.jl:87 (POLAR_MATRICES) at relative error 0.0420681631196805 against 1e-4, while test_moment_units.jl passed 17/17 in the same job. That is the open flake test_forwarddiff.jl's POLAR_MATRICES check flakes at ~4%, and the oper #287, with the same value main hit on the docs: the settings file has no reference page — mesh: and airfoil: are reachable only through docstrings #308 merge. This branch changes no code that test reaches. Re-running the job with nothing changed turned it green.
  • Docs build: not run (docstring text only, no new symbols) · REUSE: repo does not use it · rebased onto main 2b312ea
  • Benchmark: n/a
  • Risk: the wake or filament core picks up an absolute length in a later change, which would break exact scaling and make this test fail for a reason unrelated to units.

Scope

+69 / −3 across 4 files. The 60-line test file is the change; runtests.jl gets its include line; src/solver.jl changes three docstring unit labels, with the matching changelog entry.

Closes #209 · task VortexStepMethod.jl-209

1-Bort-1 and others added 2 commits September 16, 2026 20:29
Scaling every length by k at fixed inflow and polar must scale the total and
per-panel moments by k³, the per-span airfoil moment by k², and leave the
moment coefficients unchanged, on both solve! and solve. The per-span lift,
drag and moment distributions were documented in whole-panel units.

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 16, 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 · 1 inline, 0 off the diff

Good

  • The scaling test needs no reference numbers: at fixed inflow and polar, N goes as k², N·m as k³, N·m/m as k² and coefficients stay fixed, so a missing chord factor shows as a wrong power of k
  • The docstring fix is correct: panel_loads in src/panel_aerodynamics.jl:177 computes lift = cl*q*chord and moment = panel_moment(cm,q,chord) with no width, so these values are per unit span
  • The moment_dist k³ check fits src/solver.jl:424 (dot(force, z_airf)*arm + pitching_moment, both in N·m), so the test pins a real N·m quantity
  • Line 37 (all(!iszero, small.panel_moment_dist)) makes sure cm ≠ 0, so the test cannot pass on the cm = 0 case that hid the v2.3.0 bug
  • Both solve! and solve are covered, and the card says the old chord^2 → chord bug was put back by hand and caught on both
  • Scope matches the card: one test file, one include line, three docstring unit labels plus the changelog entry; no test line is longer than 92 characters (checked by grep)

Not good

  • test/solver/test_moment_units.jl:26 — This is a two-line comment block explaining why the test works; the rubric allows at most one line, and the reasoning belongs in the PR card. The testset name on line 25 already says what the test checks, so the comment can go.
  • The docstring now labels lift_dist and drag_dist [N/m], but no test checks them at k²; panel_moment_dist is the only one of the three relabelled fields the test covers
  • The map((1.0, k)) do scale ... end blocks are the same in both testsets and are closures doing real work (build, solve); one module-level helper would remove both copies (§2, §6)
  • Line 19 swaps 'around the spanwise vector of the panels' for the internal field name y_airf, which a user reading VSMSolution may not know
  • Verification is incomplete: the local CI mirror was started but its result not read, GitHub CI is pending, and the docs build was not run
  • The 'Found, not done' bullet on fail-fast: false in CI.yml doesn't say what is wrong with it or what should change, so there is nothing to act on
  • The card doesn't say what was searched for before adding scaled_wing_aero; similar wing builders already exist (test_billowing.jl build_flat_wing, test_flow_curvature.jl) (§2)

claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.

end

@testset "moments carry N·m: lengths scaled by k scale them by k³" begin
# at fixed inflow and coefficients, N scales as k², N·m as k³, and N·m per

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 a two-line comment block explaining why the test works; the rubric allows at most one line, and the reasoning belongs in the PR card. The testset name on line 25 already says what the test checks, so the comment can go.

@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 16, 2026
@1-Bort-1

1-Bort-1 commented Sep 16, 2026 •

Copy link
Copy Markdown
Member Author

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

@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!

@1-Bort-1 1-Bort-1 added agent:ci Agent task state agent:review Agent task state and removed agent:running Agent task state agent:ci Agent task state labels Sep 16, 2026
@1-Bart-1
1-Bart-1 merged commit 226fe70 into main Sep 16, 2026
11 of 12 checks passed
@1-Bart-1
1-Bart-1 deleted the agent/209-add-tests-that-check-if-moment-have-righ branch September 16, 2026 22:00
@1-Bort-1 1-Bort-1 added agent:done Agent task state and removed agent:review Agent task state labels Sep 16, 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.

Add tests that check if moment have right units

2 participants