Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
plot_geometry (and its saved views), plot_distribution, plot_combined_analysis, plot_section_polars, plot_airfoil_fit and plot_airfoils take show_title=true. With false the title is not drawn; it still names the saved file and window. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1-Bort-1
left a comment
There was a problem hiding this comment.
Independent review (advisory)
Verdict: APPROVE WITH COMMENTS · 3 inline, 0 off the diff
Good
- Covers all six plots the card names; in the extension,
plot_polarsonly usestitlefor the file and window names, andplot_slices_3donly has its hover hint, which matches the card's reasons for leaving them out show_title && Label(fig[0, …])leaves grid row 0 out entirely, so the multi-panel layouts close up instead of keeping an empty band- Section polars: MakieControlPlots'
plotxalready defaults totitle=""and sends it only to the first axis, so the empty-string route is that library's own 'no title' setting - The four copy-pasted
plot_geometrysaves became one loop over a NamedTuple, which keeps their order, file names and angles; the change is small and named in the card - The
zoom=1.8docstring now matches the real default of0.5, a bug found next to the change and named in the card - Every function gets both a true and a false case, and each checks that subplot titles stay drawn and the saved file still uses the title
- Each keyword follows its function's existing style: typed
::Boolwhere the neighbouring keywords are typed, plain=trueelsewhere
Not good
ext/VortexStepMethodMakieExt.jl:731— The loop variableviewshadowsBase.viewinsideplot_geometry;view_namewould avoid that and read better next toview_elevationandview_azimuth.test/plotting/test_plotting.jl:187— Theshow_title=truecheck saves withis_save=true, so it renders and writes four extra 1400×1400 Axis3 figures to test something that needs none; dropis_saveandsave_pathhere as the distribution case does.CHANGELOG.md:9— 'and it still names the saved file and the window where it did' is hard to read and only makes sense to someone who knows the old behaviour; 'the title still names the saved file and window' says what is true.- The generic
plot_airfoil_fitandplot_airfoilsstubs insrc/obj_adapter/ObjAdapter.jlonly saykwargs..., soshow_titleis documented only in the extension docstrings; this matches how the other keywords are handled there - The test's
drawn_titlesreadstitlevisible, so it cannot catch the layout risk the card names (Axis3 still reserving title space); the card says so openly - The
plot_section_polarstest checksplt.title(the argument passed in), not what was drawn; this is enough only because MakieControlPlots handles""itself plot_airfoilsnotes the keyword in a prose sentence while the other five use a keyword-list entry; that follows its prose-only docstring, but a reader scanning the list will miss it
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| fig = create_geometry_plot_makie(body_aero, "$(title)_side_view", 0, -90) | ||
| save_plot(fig, save_path, "$(title)_side_view", data_type=data_type) | ||
| views = (angled=(15, -120), top=(90, 0), front=(0, 0), side=(0, -90)) | ||
| for (view, (elevation, azimuth)) in pairs(views) |
There was a problem hiding this comment.
MINOR: The loop variable view shadows Base.view inside plot_geometry; view_name would avoid that and read better next to view_elevation and view_azimuth.
| @test fig isa Figure | ||
|
|
||
| @testset "show_title=false hides the title but still names the file" begin | ||
| fig = plot_geometry(body_aero, "Hidden geometry"; save_path=save_dir, |
There was a problem hiding this comment.
MINOR: The show_title=true check saves with is_save=true, so it renders and writes four extra 1400×1400 Axis3 figures to test something that needs none; drop is_save and save_path here as the distribution case does.
There was a problem hiding this comment.
Fixed in aa41be8: the show_title=true geometry call no longer saves.
|
|
||
| - `plot_geometry`, `plot_distribution`, `plot_combined_analysis`, `plot_section_polars`, | ||
| `plot_airfoil_fit` and `plot_airfoils` take `show_title=true`; with `false` the title | ||
| is not drawn, and it still names the saved file and the window where it did. |
There was a problem hiding this comment.
MINOR: 'and it still names the saved file and the window where it did' is hard to read and only makes sense to someone who knows the old behaviour; 'the title still names the saved file and window' says what is true.
There was a problem hiding this comment.
Fixed in aa41be8: 'is not drawn, and still names the saved file and window'.
… test only reads titles Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Local full suite: PASS (5 min, Julia 1.13.0, one cell of the matrix) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…hould-have-the-option-show-ti # Conflicts: # CHANGELOG.md
…tries Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TL;DR
plot_geometry,plot_distribution,plot_combined_analysis,plot_section_polars,plot_airfoil_fitandplot_airfoilstakeshow_title=true. Withfalsethe title is not drawn, so a figure can go into a paper whose caption already names it, while the title still names the saved file and window.What changed
A figure-level
Labelis only created whenshow_titleis true, so its row disappears and the layout closes up rather than leaving a blank band. A single-axis figure keeps its title on the axis withtitlevisible=show_title.plot_section_polarspasses an empty title to MakieControlPlots. Subplot titles ("CL Distribution", "Wing Geometry", "Kulfan Parameters") stay: they label panels, not the figure.plot_polarsandplot_polar_dataget no keyword, because neither draws its title (the ControlPlots version before #253 drew none either).plot_slices_3dgets none, because its title is the "hover a slice" hint of an interactive diagnostic.Cleanup inside this diff:
plot_geometry's four copy-pasted saved views are one loop over a named tuple of (elevation, azimuth), with the same file names in the same order.create_geometry_plot_makie's docstring saidzoom=1.8; the default is0.5.Verification
MethodError: ... got unsupported keyword argument "show_title"(plot_geometry: 62 passed, 1 errored; the airfoil/section-polar set: 2 passed, 2 errored).test/plotting/test_plotting.jlgreen after, rerun on the merge with main 37759d9 (juliaserver, Julia 1.13): Plotting (Makie) 72/72, section polars 19/19, airfoil skin 22/22, main's border_color 6/6, generated_slices 6/6, new airfoil/section-polar titles 8/8, audit slices 7/7### Addedlist, where both entries stay; the ext and test changes merged cleanly and touch the panel plots, not the titled plotsfunctions.mdandprivate_functions.md); not rebuilt after the merge, which adds no symbol of this branchagent ci-local, one matrix cell): PASS in 8 min on a0b5a5f · GitHub CI on a0b5a5f: PASS (Julia 1.12 on Linux, macOS, Windows; 1.13 on Linux; docs; setup; codecov/patch)titlevisible=falseon anAxis3might still reserve title space on some Makie version; the test checks that the title is not drawn, not the layout.Scope
+128 / -34 across 4 files: 74 lines of tests, 6 of changelog, 4 docstring lines in
src/VortexStepMethod.jl, and the extension, where the keyword threads through six functions and their docstrings and the geometry views fold into a loop. No REUSE setup in this repo.Opened by
1-Bort-1, an AI agent working for @1-Bart-1.Closes #223 · task
VortexStepMethod.jl-223