Conversation
…dels center_to_com!, calculate_inertia_tensor and calc_inertia_y_rotation are removed from ObjAdapter with their export, tests and docs entry. read_faces stays. Also drops the test scaffolding only they used: the serialized _info.bin, the .obj the alignment test read back, and the Serialization test dependency. 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 · 0 inline, 0 off the diff
Good
- The diff matches the card: all three functions are removed along with their export, tests and docs entry, and
read_facesstays. The commit stat is +11/−213 across 7 files, as the card says. - No uses remain: grepping
src/,test/,docs/and*.tomlfinds no reference tocenter_to_com!,calculate_inertia_tensororcalc_inertia_y_rotation. Only the untracked.agent/notes mention them. using NonlinearSolveis still needed becausefind_circle_center_and_radiuscallsNewtonRaphsonatobj_geometry.jl:89.LinearAlgebrais still needed fornormatobj_geometry.jl:211and inobj_slice.jl.- It is safe to drop
Serializationfromtest/Project.toml: no test usesserializenow, and the rootProject.toml:31still lists it, so the manifest does not change. - The cleanup in the "Interpolation Creation" testset keeps behaviour: the
faces,.objwrite and_info.binit removes are not used later in the testset. Theread_aero_matrixandcreate_interpolationsassertions are unchanged. - No test was weakened: the only removed assertions tested the deleted functions, and the existing
@test_skipis untouched. - This is removal the rubric asks for: §2 wants one source per equation and nothing kept in case, and the other copy lives in SymbolicAWEModels, which takes only
read_facesfrom here. - The
BREAKING:changelog entry goes under a new## Unreleased, and the version inProject.tomlis not bumped.
Not good
- The changelog line says the names are no longer exported and then "they are gone", which says the same thing twice. "
ObjAdapterdropscenter_to_com!, …" would be enough. - The docs build was not run locally, so the CI Documentation job is the only check. The risk is low: only two
@docsentries were removed, andcalc_inertia_y_rotationhad no docstring or docs entry. test_obj_pathis now written and read only by the "OBJ File Reading" testset. It could move inside that testset, but that is optional.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
|
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! |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
||
| ### Changed | ||
|
|
||
| - BREAKING: `ObjAdapter` no longer exports `center_to_com!`, `calculate_inertia_tensor` |
There was a problem hiding this comment.
Keep the functions, mark them deprecated, and rm them in the next breaking release, because otherwise we have too many breaking changes.
There was a problem hiding this comment.
Done in e344c8e: the three functions are back with their export, tests and docs, each emits a deprecation warning, and the changelog entry is a deprecation instead of BREAKING.
There was a problem hiding this comment.
Ok fine we are going breaking already anyways. Just mark them breaking and not backwards compatible.
There was a problem hiding this comment.
Done in 8880556: the changelog entry is BREAKING again.
There was a problem hiding this comment.
Removed in 8880556, reverting the deprecation round.
Per review, center_to_com!, calculate_inertia_tensor and calc_inertia_y_rotation come back with their export, tests and docs entry, and each now emits a deprecation warning naming SymbolicAWEModels. Their tests assert the warning with @test_deprecated. The changelog entry is a deprecation rather than BREAKING; removal waits for the next breaking release. The dead _info.bin serialization in test_kite_geometry.jl and the Serialization test dependency stay removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reverts e344c8e. Per review the next release is breaking anyway, so center_to_com!, calculate_inertia_tensor and calc_inertia_y_rotation go rather than deprecate: the export, their tests and their private_functions.md lines are removed with them, and the changelog entry is BREAKING again. SymbolicAWEModels owns the mesh inertia integral and uses read_faces, which stays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rkite-jl-onto-symbolicawemode
The file's only Interpolations names went with the inertia testsets; `I(3)` is LinearAlgebra's. Verified in a fresh process, not the warm session where an earlier include had leaked the names into Main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TL;DR
center_to_com!,calculate_inertia_tensorandcalc_inertia_y_rotationare removed fromObjAdapter, with their export, their tests and theirprivate_functions.mdentries. SymbolicAWEModels computes mesh mass properties from its own copies of the same integral, so two packages held one piece of physics and nothing would ever have reported that they had drifted apart.What changes for a caller
The three names no longer exist and are no longer exported;
read_faces, which is all SymbolicAWEModels actually takes fromObjAdapter, is untouched. The changelog entry under## Unreleased→### ChangedisBREAKING:.An earlier round of this branch deprecated the three instead, on review; review then said the next release is breaking anyway, so that round is reverted and they go.
Who uses them
Nobody. The search was repeated on 2026-09-20, immediately before this push:
gh search codefor each of the three names across OpenSourceAWE and 1-Bart-1, then a shallow clone andrgof all 43 repositories in those two accounts, private ones included. No importer. SymbolicAWEModels (src/obj_adapter.jl:50,src/system_structure/rigid_body.jl:191) and StableAWEModels (src/system_structure/rigid_body.jl:180) define their own copies and call those; SymbolicAWEModels' only call into here isObjAdapter.read_facesatsrc/obj_adapter.jl:75. BeyondTheSimexamples/neuralfoil.jl:67does a bareusing VortexStepMethod.ObjAdapter, which would have pulled the three names into scope, and calls none of them.Tests
Their testsets go with them: "Center of Mass Calculation", "Inertia Tensor Calculation" and "Alignment to principal frame" in
test_kite_geometry.jl, and "center_to_com! rejects non-triangular faces" intest_obj_adapter.jl. The "Interpolation Creation" testset also serialized an_info.binof inertia tensor, COM and interpolants that nothing ever read; that block is gone too, and with itSerializationfromtest/Project.tomlandInterpolationsfrom the test's imports — the file's last user of each. The.objwritten there stays, because "OBJ File Reading" reads it back.Found on the way, not changed here
find_circle_center_and_radiusandcreate_interpolations, the other twoObjWingleftovers inobj_geometry.jl, have no caller insrc/orext/either: only their own testsets call them. They are unexported, so deleting them is not breaking, but it wants its own importer search and belongs in its own diff.Verification
test/ram_geometry/test_kite_geometry.jl: 17 pass, 1 broken (the pre-existing@test_skip), exit 0 in a fresh--project=testprocesstest/obj_adapter/test_obj_adapter.jl: 51/51 pass ·test/Aqua.jl: 10/10 passorigin/main@ 0c94c6d (Give every untyped empty container its element type #338's typedread_faces, Build a Solver from VSMSettings or from panel and section counts, deprecate the body_aero constructors, and check the sizes in solve! #340, set_va! turns the body about a stored reference_point, and set_va!(body_aero, settings) applies yaw_rate #353), no conflictagent ci-local, Julia 1.13.0, one matrix cell): started 18:58Z, result not in when this was writtenprivate_functions.mdentries removed with their docstrings, and every remaining bare name there still resolves. The build itself is the CI Documentation job — thedocs/environment has no manifest to build from locally.Scope
+6 / −214 across 7 files against main, all of it deletion apart from the three-line changelog entry. #350 also edits
test_obj_adapter.jl, and #320 and #346 also editdocs/src/private_functions.md; every open PR touchesCHANGELOG.md. Whichever merges later may need a small textual merge.Closes #324 · task
VortexStepMethod.jl-324