Repository navigation
Test that moments come out in N·m by scaling the wing's geometry - #335
Merged
1-Bart-1 merged 2 commits intoSep 16, 2026
Merged
Conversation
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
commented
Sep 16, 2026
1-Bort-1
left a comment
Member
Author
There was a problem hiding this comment.
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_loadsin src/panel_aerodynamics.jl:177 computeslift = cl*q*chordandmoment = panel_moment(cm,q,chord)with nowidth, so these values are per unit span - The
moment_distk³ 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!andsolveare covered, and the card says the oldchord^2 → chordbug 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_distanddrag_dist[N/m], but no test checks them at k²;panel_moment_distis the only one of the three relabelled fields the test covers - The
map((1.0, k)) do scale ... endblocks 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 readingVSMSolutionmay 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: falsein 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.jlbuild_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 |
Member
Author
There was a problem hiding this comment.
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.
Member
Author
|
Local full suite: PASS (10 min, Julia 1.12.7, one cell of the matrix) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1-Bart-1
deleted the
agent/209-add-tests-that-check-if-moment-have-righ
branch
September 16, 2026 22:00
This was referenced Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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!andsolve. The v2.3.0 bug, a moment missing one chord factor, came through every existing moment test unnoticed because they all use chord = 1 orINVISCIDsections, 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_distandmoment_coeffs, sortol = 1e-6is loose. It coversforce,moment,m_body_3D,moment_dist,moment_unrefined_dist,panel_moment_dist,moment_coeffsandmoment_coeff_distfromsolve!, andMx/My/Mz,M_distributionandcmx/cmy/cmzfromsolve.Fixed on the way
VSMSolution's docstring listedlift_dist,drag_distandpanel_moment_distas [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 pinspanel_moment_distat k².Found, not done
.github/workflows/CI.yml:22hasfail-fast: false. That belongs in a separatecleanup:PR, not this one.Verification
panel_momentchanged fromchord^2tochord, 10 of 18 checks fail (7 insolve!, 3 insolve), e.g.solveMz:-9104.95 ≈ -9815.41. With* c_refdropped from the four coefficient divisions, 5 of 18 fail. Source restored after each run.test/solver/test_moment_units.jlgreen on unchanged source, 17/17 after merging the two status checks (juliaserver, fresh session on the rebased branch)agent ci-local, Julia 1.12.7): PASS in 8 min, exit 0Julia 1.12 - ubuntu-latestfailed one check outside this change:test_forwarddiff.jl:87(POLAR_MATRICES) at relative error 0.0420681631196805 against 1e-4, whiletest_moment_units.jlpassed 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 valuemainhit on the docs: the settings file has no reference page —mesh:andairfoil: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.Scope
+69 / −3 across 4 files. The 60-line test file is the change;
runtests.jlgets its include line;src/solver.jlchanges three docstring unit labels, with the matching changelog entry.Closes #209 · task
VortexStepMethod.jl-209