solve! fills VSMSolution with everything solve() returned; solve() and calculate_results are removed

[1-Bort-1]

TL;DR

VSMSolution now carries every quantity the solve() dictionary held, filled on every solve! without allocating. Plotting, generate_polar_data, the examples, docs and tests read the struct, and solve() and calculate_results are gone (BREAKING). Before this, there were two codepaths computing the same forces, and they had already drifted apart.

What changed

New fields, named after the old dictionary keys in lowercase as agreed on the thread: lift, drag, side, cl, cd, cs, cl_distribution, cd_distribution, cs_distribution, alpha_uncorrected, va_ref_vec, q_ref, rey, area_all_panels, projected_area, wing_span, aspect_ratio_projected. The two center-of-pressure fields center_of_pressure and panel_cp_locations are now filled too. Keys that already had a field map onto it: Fx… → force, Mx… → moment, cfx… → force_coeffs, cmx… → moment_coeffs, F_distribution → f_body_3D, M_distribution → m_body_3D, alpha_at_ac → alpha_dist, alpha_geometric → alpha_geometric_dist. The CHANGELOG lists this mapping for users.

calc_forces! is now the only place forces are assembled. The projections that only the dictionary had (lift/drag/side on the reference inflow, and per-panel coefficients on each panel's own inflow), the span and both centers of pressure moved into one function, analysis_fields!. It projects the f_body_3D that calc_forces! has already computed, so nothing is computed twice.

Uwe asked for a way to skip the extra fields. The existing calc_only_f_and_gamma setting now does that: it leaves them at their last value. Its only consumer was the short dictionary path of solve(), so it was about to become dead code. Giving it this meaning adds no new option and removes none. With the default (false), each solve! also computes the panel-intersection search, both centers of pressure and the projections. That adds 1.33–1.43× to calc_forces!, but only 1.8 % to a whole solve! on a 40-panel rectangle; on the V3 kite the difference is below the run-to-run noise. A simulation loop that never reads these fields can still set the flag.

Where the numbers moved

I compared the old solve() dictionary with the new solve! struct on 24 configurations: rectangle, two perpendicular wings, and a dihedral inviscid wing; × VSM/LLT; × correct_aoa; × {flow curvature, viscous drag, attached-trailed force}. That is every key in every configuration. In 18 configurations every key agrees to ≤ 2.2e-13 relative.

The other 6 are LLT with correct_aoa. solve! only ever corrected the angle of attack for VSM, while solve() corrected it for LLT too. The struct keeps the solve! behaviour, which is the one KiteModels runs. Against the old solve(), cl moves ≤ 0.34 %, cd ≤ 0.43 %, cmx ≤ 4.5 %, and alpha_at_ac changed sign on some panels of the rotated wing.

Found on the way

• Center-of-pressure warning: find_center_of_pressure warned whenever the line of action missed every panel. That happened in 7 of the 24 configurations above, and would now happen on every solve! in a simulation loop. It now leaves NaN, as documented on the field, and no longer warns. This removes the warning Warning: No intersection found with any panel in center-of-pressure calculation. #221 reports for pyramid_model.jl. The cause is the same: that model's line of action misses the canopy.
• spanwise_extent allocations: it built a generator that allocated 208 B per call. calc_forces! must stay at 0 B (existing tests assert it), so I rewrote it as a plain loop. It also runs in solve_base! for an ELLIPTIC initial circulation, so solve! on the V3 kite drops from 12 672 B to 2 080 B per call.
• Divide-by-zero guards: calculate_results threw ArgumentError on a panel with zero inflow or a body with zero area. solve! runs in simulation loops, so it does not throw there. A panel without inflow now gets NaN in its *_distribution entries, as the field docstring says. Zero total area already gave Inf in force_coeffs on main.
• solve! overwrites its solution: solve! returns solver.sol, and the next call overwrites it. plot_combined_analysis, plot_polars and generate_polar_data now leave solver.sol at their last angle. The CHANGELOG and tips_and_tricks.md say so.
• Two examples drew a stale solution: that overwrite caught V3_kite.jl and pyramid_model.jl. Both kept results = solve!(solver, …), then ran plot_polars on the same solver, and only then drew plot_distribution([results]). So the spanwise plot showed the last polar angle under the configured angle's title: V3 kite cl 0.7123 → 0.9398, with geometric α near 25° under "alpha 7.4". The dictionary had hidden this because it was a copy. e9bd10c draws the spanwise plot before the sweep in both. The other examples, the docs examples and plot_combined_analysis (spanwise panels drawn before its sweep) already did it in that order.

pushed order (8a35a07)	e9bd10c

• billowing.jl fails under GLMakie, before and after this branch: it is the only example with SAVE_ALL = true. Under GLMakie, any plot function called with is_save=true, is_show=true throws GLMakie can not display a scene in multiple Screens. It reproduces on the base 675f0e7 in a fresh session, so it is not caused here. I filed it as Plot functions throw under GLMakie when is_save and is_show are both true #398.

Where I would push back

cl_distribution (projected on the inflow, including every force term) now sits next to the existing cl_dist (airfoil coefficient at the local alpha). The docstring says which is which. Renaming either one is out of scope here.

Verification

• Reproduced first: n/a (refactor). The parity probe above is the evidence that values are unchanged.
• Red before, green after. Ported and new tests run against the base source fail with FieldError: type VSMSolution has no field cl / alpha_uncorrected, and iterate(::Nothing) for the center of pressure. They pass on this branch. New testset: solve! fills the reference inflow and the centers of pressure (reference inflow, q, Re, both centers of pressure, NaN through a gap, and calc_only_f_and_gamma keeping cl, lift and cl_distribution when the inflow changes).
• Passing (juliaserver): test_solver, test_attached_trailed_force, test_viscous_drag_correction, test_wing_directions, test_moment_units, test_forwarddiff, test_body_aerodynamics, test_results, test_unrefined_dist, test_stability, test_flow_curvature, test_verification, test_settings, test_plotting (CairoMakie) and bench.jl. The calc_forces! 0-byte assertions pass. After the review round (8a35a07) I re-ran test_solver, test_body_aerodynamics and test_moment_units and the docs build; all pass.
• Docs build clean (juliaserver, docs/make.jl, 77 s), with only the existing size-threshold warnings.
• Examples: examples/menu.jl --run-all from the repo root, with GLMakie on the box's display. Every example runs except billowing.jl, which is Plot functions throw under GLMakie when is_save and is_show are both true #398 and fails the same way on the base. After e9bd10c I re-ran V3_kite.jl and pyramid_model.jl; both ran.
• Plots, before vs after: I rendered plot_geometry, plot_distribution, plot_polars and plot_combined_analysis for the V3 kite to PNG under GLMakie, with the same inputs on the base (the solve() dictionary) and on this branch. Six of the seven PNGs are byte-identical. The seventh, plot_distribution, looks the same; its file differs by one byte because cl differs by 3e-16.
• After merging main (e493bb1), re-run in juliaserver: test_polar_vectors (new on main), test_solver, test_body_aerodynamics, test_results, test_moment_units, test_wing_directions, test_verification, test_plotting and bench.jl: all PASS. Docs build clean.
• Local CI mirror (full Pkg.test(), Julia 1.13, one cell) at e493bb1: PASS (9 min, exit 0). GitHub CI at e493bb1: PASS on all seven checks.
• Allocations (deterministic):
  • calc_forces!: 0 B → 0 B
  • solve! on a 40-panel rectangle: 1 760 B → 1 760 B
  • solve! on the V3 kite: 12 672 B → 2 080 B
• Benchmark, flag off vs on (same branch, same process, interleaved 30 rounds, pinned to one core, bench-end: conditions held):
  • solve! on a 40-panel rectangle: 156.3 µs vs 153.4 µs, ratio 1.018 (IQR 1.012–1.026)
  • solve! on the V3 kite: ratio 0.91 (IQR 0.89–0.94), i.e. the cost is below the noise
  • calc_forces! alone: 1.33–1.43×
  • Base vs branch timing is still inconclusive: five attempts, all conditions moved. The allocations above are the base-vs-branch evidence.
• The keep-last-value test fails when the flag's gate is removed from calc_forces! (cl 0.838 ≠ 0.420 at the new inflow), and passes with it.
• Risk: downstream code that reads sol.center_of_pressure === nothing, or that keeps two results from one solver and expects them to differ.

Scope

+359 / −612 across 29 files. Of this, src + ext is +183 / −415 (mostly calculate_results and solve deleted). Tests are +85 / −126: Dict reads ported to fields, the Dict-vs-struct comparisons and the calculate_results allocation benchmark deleted together with the code they covered, plus one new testset. Examples and docs are mechanical solve → solve! edits, plus the two moved plot_polars blocks above. Before adding the test helper inviscid_plates_aero, I searched test/ for inviscid rectangle helpers. The closest, flat_plate_wing, builds one wing on a stalling Breukels polar and cannot make the several gapped plates this test needs. Built on #389, now merged; e493bb1 merges main back in (#389, the KA-frame docstrings of #390 and border_color). Its conflicts were docstring and CHANGELOG only: main re-worded solve's and calculate_results' docstrings, which this branch deletes, so the removal stands and solve!'s docstring takes the KA wording. #391 (show_title on the plots) touches the same Makie functions, so whichever lands second resolves that conflict. #393 and #396 also edit calc_forces!.

Opened by 1-Bort-1, an AI agent working for @1-Bart-1.
Closes #94 · task VortexStepMethod.jl-94

[1-Bort-1]

Independent review (advisory)

Verdict: APPROVE WITH COMMENTS · 2 inline, 0 off the diff

Good

• One codepath: calculate_results and solve are deleted, and analysis_fields! only projects the f_body_3D that calc_forces! already built, so forces are not computed a second time (checked in src/solver.jl:648-689).
• The mapping from each dictionary key to its field is complete and matches the card: alpha_geometric_dist is already filled in solve_base!/calc_forces! (src/solver.jl:395-442), so the old copy-back in solve had no job left.
• calc_only_f_and_gamma now has a new meaning but is not a new option; linearize passes it through to the Dual solver (src/solver.jl:1281), so the skip also applies there.
• Deleted tests are covered elsewhere: the solve! testset in test_moment_units still checks the k³ scaling of moment, m_body_3D and moment_coeffs, and the Dict-vs-struct parity tests go away together with the second path.
• find_center_of_pressure! uses body_aero.work_vectors typed on T, so the Dual body_aero_d in linearize does not narrow Duals into Float64 buffers.
• plot_combined_analysis plots results_spanwise_list (ext line 1363) before generate_polar_data overwrites solver.sol (line 1392), so reusing solver.sol does not corrupt the spanwise plots.

Not good

• src/solver.jl:626 — With the default calc_only_f_and_gamma=false, every solve! in a KiteModels loop now runs the whole-body panel-intersection search, both center-of-pressure passes and the projections, which the removed comment kept out of solve! on purpose; the card has no timing to back this up, so either get a quiet-box number or say in the card that dynamic users should set the flag.
• test/solver/test_solver.jl:237 — The skip test only shows that the fields stay at their zero defaults on a fresh solver, which would also pass if the flag skipped too much; the ¹ contract is 'kept at its last value', and that stays untested (solve with the flag off, flip it, solve at another inflow, assert cl unchanged).
• CHANGELOG lines 25-27 are a countermand that reads backwards ("no longer corrects … as solve! already did; solve did"); state that LLT never applies correct_aoa, and give the size of the shift.
• analysis_fields! drops the old va_panel > 0 / total-area ArgumentError guards, so a panel with zero inflow now silently gives Inf/NaN in cl_distribution instead of throwing.
• compute_panel_center_of_pressures! got a ! and a new signature but no docstring, while its sibling find_center_of_pressure! got one and a place in private_functions.md.
• test/solver/test_moment_units.jl leaves an empty line before the closing end where the solve testset was removed.
• flat_plate_aero sits in the same file as the existing flat_plate_wing (test_solver.jl:271); the card does not say it searched for existing helpers, which §2 asks for.
• In test_body_aerodynamics, results_NEW and loop_sol are now the same object (loop_solver.sol), so later asserts on results_NEW read whatever the last solve! on that solver left there; it works today only because both calls use identical inputs.

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

[1-Bort-1]

MINOR: With the default calc_only_f_and_gamma=false, every solve! in a KiteModels loop now runs the whole-body panel-intersection search, both center-of-pressure passes and the projections, which the removed comment kept out of solve! on purpose; the card has no timing to back this up, so either get a quiet-box number or say in the card that dynamic users should set the flag.

[1-Bort-1]

Measured in 8a35a07's round, quiet window (bench-end: conditions held), flag off vs on in one process: solve! on a 40-panel rectangle 1.018× (IQR 1.012–1.026), on the V3 kite below the noise; calc_forces! alone 1.33–1.43×. The numbers are in the card, which also says a loop that never reads these fields can set calc_only_f_and_gamma.

[1-Bort-1]

MINOR: The skip test only shows that the fields stay at their zero defaults on a fresh solver, which would also pass if the flag skipped too much; the ¹ contract is 'kept at its last value', and that stays untested (solve with the flag off, flip it, solve at another inflow, assert cl unchanged).

[1-Bort-1]

Fixed in 8a35a07: the test solves with the flag off, turns it on, solves at 10° instead of 5° and asserts cl, lift and cl_distribution are unchanged while force grew. It fails with the gate removed (cl 0.838 vs 0.420).

[1-Bort-1]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[1-Bart-1]

Did you locally build the docs and make sure they still work? And run the examples? And check if the plotting functions still work? GLMakie has display on the box.

[1-Bart-1]

See comment

[1-Bort-1]

Done: docs build clean, all examples run except billowing.jl (fails the same way on the base: #398), plots match the base; the stale spanwise plot the run turned up is fixed in e9bd10c.

[1-Bort-1]

Yes to all three. The docs build is clean. Every example runs under GLMakie except billowing.jl, which fails the same way on main (#398). The V3 plots render the same before and after. Running the examples turned up a bug: V3_kite.jl and pyramid_model.jl drew the 25° polar-sweep solution under the configured angle. Fixed in e9bd10c.