Conversation
…c length station_indices now places its targets along the quarter-chord line with the chordwise component dropped, so a tip whose leading edge runs aft no longer gathers sections, and wingtip_distance is a spanwise inset. march_edges drops its arclen field, which only station_indices read. 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 · 1 inline, 0 off the diff
Good
- The fix matches the card:
station_indicesnow spreads targets over the quarter-chordhypot(Δy, Δz)length, and themin_chord_fractrim andwingtip_distanceuse that same length (obj_slice.jl:464-473). - Removing
arclenfrommarch_edgesis safe: a repo-widerg arclenfinds no other reader, andplot_slices_3din the Makie extension only callsstation_indices. - The docs are consistent: settings.jl, settings.md, airfoil_pipeline.md, both docstrings and the changelog all call
wingtip_distancea spanwise length [m], andrg 'arc length'finds nothing stale in the slicer path. - The new test checks both even spacing and the size of the inset; I worked it through by hand: all 61 stations stay above
min_chord_frac, the targets land within one 0.05 m station of the expected spacing, and the tolerance holds. - The change stays in scope: the only tidy-up is rewrapping the
obj_to_yamldocstring, as the card says, andmin_chord_fracpassthrough and #272/#273 are left out on purpose.
Not good
src/obj_adapter/obj_slice.jl:468— The card says this matches whatrefine_mesh_for_linear_cosine_distribution!measures, but that function (andcompute_refined_section_interpolation!) uses full 3D quarter-chord length withxincluded. The repo now has two different ideas of span position, and refinement still places panels by the length this PR stops using. Either correct the card and say so, or explain why the two may differ.- The test has z = 0 everywhere, so it only checks that
xis dropped. Changing the code to use plainywould still pass, even though the card's reason for includingzis the curled ram-air tip. - If closed tip stations step only in
x, theirspanvalues equalspan[first(usable)].argminthen returns the first of the tied indices, which is a closed station, and that silently undoes themin_chord_fractrim on the left tip. This is unlikely on real meshes but worth a line in the card. - The
station_indicesdocstring is 6 lines, above the rubric's 1-4. Its last two sentences could be cut down to state only what is left out and whatwingtip_distancedoes.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| span = zeros(length(quarter_chord)) | ||
| for i in 2:length(span) | ||
| step = quarter_chord[i] .- quarter_chord[i-1] | ||
| span[i] = span[i-1] + hypot(step[2], step[3]) |
There was a problem hiding this comment.
MINOR: The card says this matches what refine_mesh_for_linear_cosine_distribution! measures, but that function (and compute_refined_section_interpolation!) uses full 3D quarter-chord length with x included. The repo now has two different ideas of span position, and refinement still places panels by the length this PR stops using. Either correct the card and say so, or explain why the two may differ.
|
Local full suite: PASS (10 min, Julia 1.12.7, one cell of the matrix) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
TL;DR
station_indicesnow places then_sectionstargets evenly along the quarter-chord line with its chordwise (x) step dropped, instead of along the leading edge's arc length. At a raked or closing tip the leading edge runs almost straight aft, so arc-length targets piled into the last few centimetres of span, andwingtip_distancegave almost no inset there.What was wrong
march_edgessummed the full 3D leading-edge step intoarclen, andstation_indicesspread its targets over that. A station where the leading edge moves 40 mm aft for 3 mm of span counts as 40 mm, so the tip cap takes a share of the sections far beyond its span.The SK100 mesh from the issue isn't in the repo, so I reproduced the problem on a synthetic march: a straight leading edge over |y| ≤ 1 m, then 20 cap stations per side running 40 mm aft per 3 mm of span (half-span 1.06 m). With 9 sections, main gives:
A 0.3 m
wingtip_distancemoved the outermost section 2 cm. That is the "1.2 m of tip inset gives 4.9 cm of span" from the issue.What changed
station_indicescomputes the quarter-chord point of every marched station and adds uphypot(Δy, Δz)between neighbours. The targets, themin_chord_fractrim andwingtip_distanceall use that length now.march_edgesno longer returnsarclen.station_indiceswas its only reader.docs/src/airfoil_pipeline.md,docs/src/settings.mdand theWingSettingsfield doc now describewingtip_distanceas a spanwise length [m]. The changelog entry is under Changed.Why not plain
y, the issue's first suggestion? The repo's ram-air kite curls down at the tips. Near station 80 the leading edge movesdy=0.0185, dz=-0.1206per step. Placing sections byywould spread the outer ones far apart along the real span. Dropping onlyxhandles both a curved span and a raked tip.Why the quarter chord? It is the line the bound vortex lies on, and it is what
refine_mesh_for_linear_cosine_distribution!already measures. Droppingxremoves the part of each step that points along the freestream, which is what made those panels degenerate.Effect on the repo's ram-air kite
The "before" panel ran main's
obj_slice.jl(loaded fromorigin/main) on the same march, with the samen_sections=10,wingtip_distance=0and axes. The largest panel span divided by the smallest drops from 1.44 to 1.10. This mesh has no long raked cap, so the change is modest here. The tips that close to a point are still trimmed as before (tip chord 0.168 m in both).Where I'd push back
n_sectionsgets close to the number of stations, rounding dominates. On this mesh,n_sections=45with the defaultn_bins=60gets worse: the span ratio goes from 2.07 to 4.45, because the curled tip stations are about 0.12 m apart iny-zwhile the inboard ones are 0.055 m apart. Withn_bins=240it improves instead, from 1.90 to 1.36. Placing targets finer than the station spacing would mean interpolating between stations, which is a separate change.d_arc=0.0816, dx=0.0807, dy=0.0032) leave about 12 mm ofzper station. If thatzis the nose sliding vertically as the section closes, rather than real span, those stations still count about 3.8× theirystep (it was 25.9×). Someone with that mesh should rerun the panel-sweep table.wingtip_distancevalues and generated geometries already out there were placed the old way.obj_to_yamlreuses an existinggeometry.yaml, andtest/generated/is keyed on settings rather than on the code, so both have to be regenerated.Left out
min_chord_fracthrough toWingSettings. The issue calls it independent, and it would add a new setting. With a spanwisewingtip_distanceit may no longer be needed; I'd open it separately if it still is.z_airfconditioning on near-streamwise panels. That is Panel normal z_airf is built from the leading-edge step, not the quarter-chord step the bound vortex uses #272 / Take the panel normal from the quarter-chord step #273.Verification
FieldError: type NamedTuple has no field arclen; witharclensupplied it fails on the numbers.test/obj_adapter/test_obj_adapter.jl"station_indices spreads sections evenly in span past a raked tip": green after the change (3/3, juliaserver)test/generated/rebuilt by this code:obj_adapter/test_obj_adapter.jl55/55 ·settings/test_settings.jl33/33 ·test_refinement_validation.jl15/15 ·wake/test_wake.jl3/3 ·body_aerodynamics/test_results.jl30/30 ·solver/test_forwarddiff.jl7/7plotting/test_plotting.jl(callsstation_indicesthroughplot_slices_3d): 58/58 in the local CI mirrordocs/make.jl) · up to date with main · no REUSE lint in this repoPkg.test()on Julia 1.12.7: PASS, 6421 pass and 1 broken (the existing@test_skipintest_kite_geometry.jl), 14 min · GitHub CI: runs when this PR openswingtip_distanceget a different inset.Scope
+56 / −33 across 7 files.
obj_slice.jlis about net zero:arclencomes out ofmarch_edgesand the spanwise sum goes intostation_indices. Also in the diff: the new test (+19), the changelog (+4), and docstring and doc wording. Rewrapping theobj_to_yamldocstring paragraph is the only tidy-up. I searched forarclen,station_indices,wingtip_distance, "arc length" and quarter-chord helpers; none existed to reuse.Closes #275 · task
VortexStepMethod.jl-275