Conversation
examples/readme_figure.jl renders docs/TU_Delft_V3_Kite.png. plot_combined_analysis now honours is_save, spans its title over both columns, frames the geometry, and dashes literature polars; CS round-off noise plots flat in both polar figures. 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
- The save fix is real:
is_save,save_pathanddata_typewere accepted but never used, and the new test assertsCombined_Analysis.pngexists; the card reports it red before the fix. - The literature dashing reuses the
idx <= n_solversrule thatplot_polarsalready uses (ext lines 978-980 and 1030-1032), so both functions style literature the same way. widen_flat_ylims!is used in bothplot_polarslayouts and inplot_combined_analysis, so there is one helper and not three inline copies. The test covers the noise case, NaN filtering, and leaving a wide range alone.- The dropped spanwise labels were only there for legends, and the only legend is built from
ax_gamma(ext line 1402). The card names this as cleanup and says the PNG comes out byte-identical. - The spanwise force loop becomes one
enumerate((ax_fx, ax_fy, ax_fz))with no string building. The file gets shorter around the change. - The old PNG is deleted, and no tracked
.mdor.jlfile still refers tov3_example_output.png. - The card says why
readme_figure.jlis not inmenu.jl:run_all()would rewrite a tracked file.
Not good
examples/readme_figure.jl:22— Lines 11-33 copy the setup at the top ofexamples/V3_kite.jlalmost line for line: the literature paths, the settings load, the geometry path fix, the wing/refine/body/solver steps and the solve. §2 counts an 80%-same copy as duplication, and a later change to the V3 setup will now need to be made in two files.examples/readme_figure.jl:31— The reference point[0.422646, 0.0, 9.3667]is a bare literal.V3_kite.jlnames the same valueREFERENCE_POINT, so the two can drift apart and nothing here says what the numbers are.- The marker, markersize and linestyle ternary on
n_solversnow appears three times in the ext. A smallsolver_or_literature_style(idx, n_solvers)would give the rule one source. - The
widen_flat_ylims!docstring clause "so that round-off noise on a zero quantity plots flat" is rationale (§4). The first sentence already says what the function does. - The test comment "CS of a symmetric wing is round-off noise; it must plot flat" does the job the testset name should do (§8).
- The CHANGELOG says "1e-16 noise" but the card says ±1e-15. Say "round-off noise" and give no magnitude.
- The spanwise panels no longer show total CL/CD/ΣF anywhere, which the reader of the README figure can see. The card leaves this to the reviewer, but the CHANGELOG does not mention it.
- The card does not say what was searched before adding
widen_flat_ylims!(§2 asks for it). - The committed PNG is 1.05 MB, about 2.5× the old one. That is acceptable for a README figure, but the card leaves the resolution choice open.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| labels = ["VSM Julia", "CFD Re=5e5", "CFD Re=10e5", "VSM Python Re=5e5", | ||
| "Wind tunnel Re=5e5"] | ||
|
|
||
| settings = VSMSettings(joinpath(project_dir, "data", "TUDELFT_V3_KITE", |
There was a problem hiding this comment.
MINOR: Lines 11-33 copy the setup at the top of examples/V3_kite.jl almost line for line: the literature paths, the settings load, the geometry path fix, the wing/refine/body/solver steps and the solve. §2 counts an 80%-same copy as duplication, and a later change to the V3 setup will now need to be made in two files.
There was a problem hiding this comment.
Fixed in cee5984: the V3 setup now lives in examples/V3_kite_setup.jl, included by both V3_kite.jl and readme_figure.jl; the re-rendered PNG is byte-identical.
| body_aero = BodyAerodynamics([wing]) | ||
| VortexStepMethod.reinit!(body_aero) | ||
| solver = Solver(settings) | ||
| solver.reference_point .= [0.422646, 0.0, 9.3667] |
There was a problem hiding this comment.
MINOR: The reference point [0.422646, 0.0, 9.3667] is a bare literal. V3_kite.jl names the same value REFERENCE_POINT, so the two can drift apart and nothing here says what the numbers are.
There was a problem hiding this comment.
Fixed in cee5984: the value is REFERENCE_POINT in examples/V3_kite_setup.jl, the one source for both scripts.
… the polar style one source Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…figures-in-the-readme # Conflicts: # CHANGELOG.md
|
Local full suite: PASS (13 min, Julia 1.13.0, one cell of the matrix) |
TL;DR
The README figure was an old
plot_combined_analysisrender (its legends sat inside the axes, over the lines), and no script in the repo could rebuild it.examples/readme_figure.jlnow rendersdocs/TU_Delft_V3_Kite.pngfrom the current Makie extension. The layout faults that render showed are fixed in the extension.What was wrong
The old
docs/v3_example_output.pngdates from 5ac1f09 (Feb 2026). It drew the spanwise and polar legends asaxislegends inside the axes, over the curves. The extension has since moved those legends below the grids, but nobody re-rendered the image, because no script produced it:examples/V3_kite.jldraws three separate figures.Rendering the V3 kite with the current
plot_combined_analysis(the "before" below) showed four more faults:Label(fig[0, :])runs before the second column exists, so the title centres over the left column only.set_axes_equal_makie!(...; zoom=0.5)doubles the axis box, so the kite fills about a quarter of it.plot_polars.plot_polarsdashes them.What changed
examples/readme_figure.jlcallsplot_combined_analysison the V3 kite with the four literature CSVs, renders with CairoMakie at a fixed 1800×1200 and saves throughis_save. Rerunning it gives a byte-identical PNG.REFERENCE_POINT) moved from the top ofV3_kite.jlintoexamples/V3_kite_setup.jl, which both scripts include, so the README figure cannot drift from the example.docs/TU_Delft_V3_Kite.png, the file namesave_plotderives from the title, and says which script renders it. The old PNG is deleted.plot_combined_analysistookis_save,save_pathanddata_typebut never saved; it now callssave_plot.zoom=1.0, and literature polars are dashed. A privatepolar_style(idx, n_solvers)now holds the solver/literature marker, size and line style that bothplot_polarslayouts andplot_combined_analysishad spelled out inline.widen_flat_ylims!(ax, series; min_span=0.1)gives the CS axis at least a 0.1 span. It is used by both layouts ofplot_polarsand byplot_combined_analysis. A CS that really varies by more than 0.1, as in a sideslip sweep, is left alone. Before adding it I searched the ext andsrc/plotting_helpers.jlforylims!,limits,extremaandautolimits: the only limit helper isset_axes_equal_makie!, for 3D geometry.CL: …/CD: …/ΣF…labels on the other eight axes were never shown. They are removed; the committed PNG is byte-identical with and without them.Before → after
Both use the same script, inputs and figure size on
origin/main: the "before" isorigin/main's ext, the "after" is this branch's.Where I'd push back
mainthey were already only in labels no legend drew, so no user-visible output changes and the CHANGELOG says nothing. If you want them back, the axis titles are the place, not legends over the curves.px_per_unit=1would halve it if size matters more.readme_figure.jlis deliberately not inexamples/menu.jl, becauserun_all()would then rewrite a tracked file.Verification
origin/main's ext (the "before" figure). The title is off-centre, the kite is small and CS is scaled to ±1e-15.test/plotting/test_plotting.jl: red before the fix, green after (juliaserver).isfile(".../Combined_Analysis.png")), 1 errored (widen_flat_ylims!).readme_figure.jlrenders a PNG byte-identical to the committed one, so the shared setup andpolar_stylechange nothing visible.origin/main(a324968, resolving a CHANGELOG conflict with march_edges takes each station's tangent from its neighbours, so no cut near a closing tip grazes the surface #376). On the merge, the plotting tests are green again (61/61, 19/19, 22/22, 7/7), andreadme_figure.jlre-renders the committed PNG byte-identically.Pkg.test) on the merge 38dd6aa: PASS in 11 min.min_span=0.1also widens a CS that genuinely varies by less than 0.1. It stays centred and visible, just flatter.Scope
+141 / −102 across 10 files, two of them the PNG swap:
ext/: the newwiden_flat_ylims!andpolar_stylewith docstrings, the save call and the dropped labels. The three inline style blocks it replaces make the file shorter overall.examples/: the new 29-linereadme_figure.jland the 24-lineV3_kite_setup.jl, with the setup taken out ofV3_kite.jl.test_plotting.jl, a changelog entry, two docs lines, two README lines and the PNG swap.Closes #381 · task
VortexStepMethod.jl-381