Give every untyped empty container its element type - #338
Merged
Merged
Conversation
`vertices = []`, `plots = []`, `filaments_plot = []` and the like become
`Vector{Float64}[]`, `Makie.AbstractPlot[]`, `Tuple{...}[]`, in src, the Makie
extension, tests and the stall example. `plot!(ax, body)` drawn as flat panels
now appends each panel's plots, so every branch returns one flat
`Vector{Makie.AbstractPlot}`.
Refs #147
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1-Bort-1
commented
Sep 16, 2026
1-Bort-1
left a comment
Contributor
Author
There was a problem hiding this comment.
Independent review (advisory)
Verdict: APPROVE WITH COMMENTS · 1 inline, 0 off the diff
Good
- The commit's own diff (HEAD~1..HEAD) is 11 files, +60/−52, and matches the card's file list and scope exactly
- A search of src, ext, test, examples and docs for
= [],Any[],([]and, []finds nothing left, which backs the card's claim - The static-body change from
push!toappend!is correct: the Panelplot!returns its own list (mesh + border), so the old result was nested and the new one is flat like the observable and airfoil branches; there is a changelog line and a test (length == 2 * panels) - The declared element types match what gets pushed:
generate_polar_databuildspolar_datafrom vectors plus distributions, and the thesis helpers'ringvec/wingpanelsdicts hold onlyVector{Float64}values calculate_filaments_for_plottingnow returns the tuple type its docstring already promised, and a new unit test pins it rather than a slow integration test- The
plot_polars(Solver[], BodyAerodynamics[], …)test calls still fit the untypedsolver_list/body_aero_listsignature, so no API changed - Deferring the
va/*_arrayrenames to #336 and #337 keeps this PR to one idea, as §1 asks
Not good
examples/stall_model.jl:35— Withrib_listgone, a stray blank line now separatesdf = CSV.read(...)from its only use, and the# Create wing geometrycomment now also sits over the CSV read loop. Moving the read down next to the loop would keep reading the data and building the wing in one place.- The mixed
PANEL_MESH_OBSERVABLESkey ((body_id, panel_idx)vs. a bare panelobjectid) is a real inconsistency found on the way, but it is only described under 'Left as they are' with no linked issue, which §5 asks for NamedTuple{(:cmx, :cmy, :cmz), NTuple{3, Vector{Float64}}}[]at ext line 934 is a long inline type; it is correct, but a reader has to parse it to see it just means 'cm columns'- Line 282 of
thesis_oriol_cayon.jlrepeats theDict{String, Any}[]literal from line 127, so the two resets have to be kept in step by hand - The only checks listed are local and filtered, and the local CI mirror had not finished; the full suite and GitHub CI are still unconfirmed
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| push!(rib_list, (LE, TE, LEI_AIRFOIL_BREUKELS, | ||
| lei_poly_coeffs(row.d_tube, row.camber))) | ||
| end | ||
|
|
Contributor
Author
There was a problem hiding this comment.
MINOR: With rib_list gone, a stray blank line now separates df = CSV.read(...) from its only use, and the # Create wing geometry comment now also sits over the CSV read loop. Moving the read down next to the loop would keep reading the data and building the wing in one place.
This was referenced Sep 16, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
4 tasks
1-Bart-1
approved these changes
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
vertices = [],plots = [],filaments_plot = []and the other bare[]/Any[]literals now say what they hold: in src, the Makie extension, tests and the stall example. This is the first box of #147. Typing the list in the Makie extension surfaced a real inconsistency:plot!(ax, body)drawn as flat panels returned one list per panel, while every other branch returned a flat list. Now all branches return one flatVector{Makie.AbstractPlot}.What changed
read_facesbuildsVector{Vector{Float64}}/Vector{Vector{Int64}}, andcalculate_filaments_for_plottingbuilds theVector{Tuple{Vector{Float64}, Vector{Float64}, String}}its docstring already promised. Both used to returnVector{Any}.plot!methods collect intoMakie.AbstractPlot[]. The static body path now usesappend!instead ofpush!, because pushing a panel's plot vector into a typed list throws. That is the one behaviour change and it has a changelog line.plot_polarstypes itspolar_data_listandcm_data_list.VSMSettings.wingsdefaults toWingSettings[]instead of a[]that kwdef converted anyway.test/thesis_oriol_cayon.jlgetDict{String, Any}[],Vector{Float64}[]and so on. Threepush!loops became comprehensions, andplot_polars(Any[], Any[], …)becameplot_polars(Solver[], BodyAerodynamics[], …).examples/stall_model.jladds each rib to the wing as it reads the CSV row, instead of filling an untypedrib_listfirst.Searched for
= [],Any[]and bare[]literals across src, ext, test, examples, docs/src, scripts and README. None are left.Left as they are
Vector{Any}[]rows ingeometry_gen.jl,obj_to_yaml.jlandSurfplanAdapter.jl. These are YAML table rows that mix anIntid withFloat64coordinates. Typing them asFloat64would print the id as3.0.Tuple[],NamedTuple[]andRef{Any}: these are typed, only abstractly.Dict().PANEL_MESH_OBSERVABLESis keyed by(body_id, panel_idx)in two places and by a bare panelobjectidin the third (plot(panel)). The mixed key is the thing to fix there, and it is a different concern from typed vectors.The rest of #147
The two renames touch about 900 names, and
va=/v_a=are public keywords, so they are queued as a stack on top of this PR:*_arraylocals to say whether they are a vector or a... #336:*_arraylocals to vector/matrix names (no public API)va,v_a,_vaandva_*... #337: onevaconvention. It starts by asking whether the publicva=keyword,set_va!andPanel.vafollow the convention agreed on Refactoring part III #147.Not stacked on #325, which also edits
obj_geometry.jlandtest_kite_geometry.jl, but in hunks that don't overlap these.Verification
Any[[0.0, 0.0, 0.0], …] isa Vector{Vector{Float64}},Any[(…, "magenta"), …] isa Vector{Tuple{…}},Any[Any[Mesh{…}, …]] isa Vector{Makie.AbstractPlot}and20 == 40(nested per-panel list)test/panel/test_panel.jl,test/ram_geometry/test_kite_geometry.jl: 3 failed before, green after.test/plotting/test_plotting.jl: 3 failed before, green after.runtests.jlfiltered to panel, kite geometry, settings, obj adapter, body aerodynamics and plotting: 5085 passed, 1 broken (the existing@test_brokenin Kite Geometry).yaml_geometry(includes wing deformation): 178 passed.examples/stall_model.jlruns to the end.julia +1.12 --project=test, not juliaserver: the box's REPL sweeper stopped this task's session every minute (1-Bart-1/Agents#413)plot!(ax, body)asplots[i]per panel now gets individual plots.Scope
+60 / −52 across 11 files. The growth is the three new type checks and the changelog line; the rest is one-for-one replacements.
Refs #147 · task
VortexStepMethod.jl-147