Skip to content

Render the README's V3 kite figure from a script with the current plot_combined_analysis, and fix that figure's layout - #382

Open
1-Bort-1 wants to merge 4 commits into
mainfrom
agent/381-update-the-figures-in-the-readme
Open

1-Bort-1 wants to merge 4 commits into
mainfrom
agent/381-update-the-figures-in-the-readme

Conversation

@1-Bort-1

@1-Bort-1 1-Bort-1 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

TL;DR

The README figure was an old plot_combined_analysis render (its legends sat inside the axes, over the lines), and no script in the repo could rebuild it. examples/readme_figure.jl now renders docs/TU_Delft_V3_Kite.png from the current Makie extension. The layout faults that render showed are fixed in the extension.

What was wrong

The old docs/v3_example_output.png dates from 5ac1f09 (Feb 2026). It drew the spanwise and polar legends as axislegends 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.jl draws three separate figures.

Rendering the V3 kite with the current plot_combined_analysis (the "before" below) showed four more faults:

  • Title off-centre. Label(fig[0, :]) runs before the second column exists, so the title centres over the left column only.
  • Kite too small. set_axes_equal_makie!(...; zoom=0.5) doubles the axis box, so the kite fills about a quarter of it.
  • CS axis scaled to noise. On a symmetric wing CS is zero up to round-off, and the axis autoscales to ±1e-15 noise. The same happens in plot_polars.
  • Literature drawn like solver output. Literature polars are solid, like the solver's; plot_polars dashes them.

What changed

  • examples/readme_figure.jl calls plot_combined_analysis on the V3 kite with the four literature CSVs, renders with CairoMakie at a fixed 1800×1200 and saves through is_save. Rerunning it gives a byte-identical PNG.
  • The V3 setup (literature paths, settings, wing, body, solver, REFERENCE_POINT) moved from the top of V3_kite.jl into examples/V3_kite_setup.jl, which both scripts include, so the README figure cannot drift from the example.
  • The README points at docs/TU_Delft_V3_Kite.png, the file name save_plot derives from the title, and says which script renders it. The old PNG is deleted.
  • Fixed: plot_combined_analysis took is_save, save_path and data_type but never saved; it now calls save_plot.
  • The title now spans both columns, the geometry uses zoom=1.0, and literature polars are dashed. A private polar_style(idx, n_solvers) now holds the solver/literature marker, size and line style that both plot_polars layouts and plot_combined_analysis had spelled out inline.
  • A new private 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 of plot_polars and by plot_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 and src/plotting_helpers.jl for ylims!, limits, extrema and autolimits: the only limit helper is set_axes_equal_makie!, for 3D geometry.
  • Cleanup: the spanwise legend is built from the Γ axis only, so the 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" is origin/main's ext, the "after" is this branch's.

before: origin/main's plot_combined_analysis on the V3 kite, same script

after: this branch, same script and inputs

Where I'd push back

  • The spanwise panels no longer print total CL/CD/ΣF anywhere. The old README image showed them in its in-axis legends, but on main they 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.
  • The PNG is 3600×2400 (CairoMakie's default 2× pixels), 1.05 MB against the old 417 KB. I kept 2× so the figure stays sharp on high-DPI screens; px_per_unit=1 would halve it if size matters more.
  • readme_figure.jl is deliberately not in examples/menu.jl, because run_all() would then rewrite a tracked file.

Verification

  • Reproduced first: rendered the V3 kite with 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).
    • Red: 14 passed, 1 failed (isfile(".../Combined_Analysis.png")), 1 errored (widen_flat_ylims!).
    • Green: Plotting (Makie) 61/61, section polars 19/19, airfoil skin 22/22, audit slices 7/7. Re-run after the review fixes (cee5984).
  • After the review fixes, readme_figure.jl renders a PNG byte-identical to the committed one, so the shared setup and polar_style change nothing visible.
  • Merged 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), and readme_figure.jl re-renders the committed PNG byte-identically.
  • Docs build: GitHub's Documentation job PASS on 38dd6aa. Not run locally, because the docs env has no manifest here.
  • GitHub CI on the merge 38dd6aa: PASS on Julia 1.12 (ubuntu, windows, macOS) and 1.13 (ubuntu), setup test and codecov/patch.
  • Local CI mirror (Julia 1.13, full Pkg.test) on the merge 38dd6aa: PASS in 11 min.
  • Risk: min_span=0.1 also 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 new widen_flat_ylims! and polar_style with docstrings, the save call and the dropped labels. The three inline style blocks it replaces make the file shorter overall.
  • examples/: the new 29-line readme_figure.jl and the 24-line V3_kite_setup.jl, with the setup taken out of V3_kite.jl.
  • A testset in test_plotting.jl, a changelog entry, two docs lines, two README lines and the PNG swap.

Closes #381 · task VortexStepMethod.jl-381

1-Bort-1 and others added 2 commits September 22, 2026 14:05
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 1-Bort-1 added agent:running Agent task state agent:ci Agent task state and removed agent:running Agent task state labels Sep 22, 2026

@1-Bort-1 1-Bort-1 left a comment

Copy link
Copy Markdown
Contributor 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 · 2 inline, 0 off the diff

Good

  • The save fix is real: is_save, save_path and data_type were accepted but never used, and the new test asserts Combined_Analysis.png exists; the card reports it red before the fix.
  • The literature dashing reuses the idx <= n_solvers rule that plot_polars already uses (ext lines 978-980 and 1030-1032), so both functions style literature the same way.
  • widen_flat_ylims! is used in both plot_polars layouts and in plot_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 .md or .jl file still refers to v3_example_output.png.
  • The card says why readme_figure.jl is not in menu.jl: run_all() would rewrite a tracked file.

Not good

  • examples/readme_figure.jl:22 — 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.
  • examples/readme_figure.jl:31 — 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.
  • The marker, markersize and linestyle ternary on n_solvers now appears three times in the ext. A small solver_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.

Comment thread examples/readme_figure.jl Outdated
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",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread examples/readme_figure.jl Outdated
body_aero = BodyAerodynamics([wing])
VortexStepMethod.reinit!(body_aero)
solver = Solver(settings)
solver.reference_point .= [0.422646, 0.0, 9.3667]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cee5984: the value is REFERENCE_POINT in examples/V3_kite_setup.jl, the one source for both scripts.

@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 22, 2026
… the polar style one source

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@1-Bort-1 1-Bort-1 mentioned this pull request Sep 22, 2026
4 tasks
@codecov

codecov Bot commented Sep 22, 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:queued Agent task state agent:running Agent task state and removed agent:running Agent task state agent:queued Agent task state labels Sep 22, 2026
…figures-in-the-readme

# Conflicts:
#	CHANGELOG.md
@1-Bort-1

1-Bort-1 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

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

@1-Bort-1 1-Bort-1 added agent:queued Agent task state agent:running Agent task state agent:ci Agent task state agent:review Agent task state and removed agent:running Agent task state agent:queued Agent task state agent:ci Agent task state labels Sep 22, 2026
@1-Bort-1
1-Bort-1 requested a review from 1-Bart-1 September 22, 2026 14:03

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:review Agent task state

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update the figures in the readme

1 participant