cleanup: apply_heading and the two frames the placement fix stopped us - #314
Open
1-Bort-1 wants to merge 1 commit into
Open
cleanup: apply_heading and the two frames the placement fix stopped us#3141-Bort-1 wants to merge 1 commit into
1-Bort-1 wants to merge 1 commit into
Conversation
…ding 510a423 replaced tether-frame placement with min_rotation and made apply_heading! measure the reference body's own orientation; neither change removed what it made dead. apply_heading(vec, R_t_to_w, curr_R_t_to_w, heading) had no callers. Its two matrices were still built by apply_azimuth_elevation! and threaded through reinit! and reposition! into apply_heading!, whose body reads neither, so both calc_R_t_to_w calls existed only to fill an unread return value. calc_R_t_to_w stays live through calc_heading and transform_pos is still what min_rotation rotates onto, so placement and heading are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3umGJ7XaayLty6sMF1L75
Contributor
Author
|
CI: https://github.com/OpenSourceAWE/SymbolicAWEModels.jl/actions/runs/34405402473/job/102647056275 |
Contributor
Author
|
Local full suite: PASS (29 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! |
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
510a4232replaced tether-frame placement withmin_rotationand madeapply_heading!read the body's own orientation, and neither change took its scaffolding out. This drops the now-deadapply_headinghelper and the two rotation matricesapply_azimuth_elevation!still built, returned and handed to a callee that never looked at them.What was left behind
apply_heading(vec, R_t_to_w, curr_R_t_to_w, heading)had no callers.grep -rn apply_headingover the repo found exactly three hits: the definition, the file's header list, and the@docsentry ondocs/src/private_functions.md.apply_azimuth_elevation!computedcurr_R_t_to_w = calc_R_t_to_w(rel_pos)andR_t_to_w = calc_R_t_to_w(transform_pos)and returned the pair;reinit!andreposition!each destructured it and threaded both intoapply_heading!. The body ofapply_heading!reads neither — since510a4232it takes its orientation fromheading_reference_bodyand rotates aboutnormalize(reference_body.pos_w - base_pos). So the twocalc_R_t_to_wcalls existed only to fill a return value nobody consumed, and the docstring line "Returns(curr_R_t_to_w, R_t_to_w)for use in that step" described a contract that had stopped being one.What is gone
The helper and its docs entry; the two
calc_R_t_to_wcalls; the return tuple and that docstring line; the two parameters onapply_heading!and the destructuring at both call sites. Nothing else changes:calc_R_t_to_wstays live throughcalc_heading(andtest_heading_calculation.jlexercises it directly),transform_posis still whatmin_rotationrotates onto, androtate_around_zis still used a few lines below. The file header's function list now namesapply_heading!, the one that exists.No
CHANGELOG.mdentry: nothing observable changes.apply_headingis private and had no callers, and the deleted return values had no readers.The red job is not this diff, and I did not touch it
Julia 1.12 - ubuntu-latest - x64 - monolitherrored intest/test_twist_alignment.jl:123withAssertionError: VSM solve failed (non-converged or non-finite) on wing 1— 3143 passed, 0 failed, 1 errored. The other five jobs of that run,Julia 1.11 - ubuntu,Julia 1.12 - macOS, the kernel-backend cell on the very same ubuntu 1.12 image, Documentation and reuse-lint, are green, and so was the full suite on the box.The deletion is inert on that test, measured rather than argued: I ran
test/test_twist_alignment.jlhere, then put the parent commit'ssrc/system_structure/transforms.jlback withgit checkout c3b2b562^ -- …and ran it again in the same session. Static twists[-0.1336630477312238, -0.0008835202645692225, -0.1319843929300836]and dynamic[0.9221199567195998, -0.0033309221603231233, 0.7965616094657846], identical to the last bit on both. It has to be: the two deletedcalc_R_t_to_wcalls are pure (smooth_normalizeand a cross product,src/generate_system/helpers.jl:210) and nothing read their results.What the test does is ramp steering for 60 steps and assert on the state it lands in, which its own comment says is next to a divergence: "Holding this steering runs the station twist away until the VSM solve diverges, so a settled operating point does not exist for this 2-plate config." Same code, four platforms, end states from 0.22 to 1.05 and once past the assertion in
vsm_aero_coeffs. It last went red the same way onfeat/point-flap-delta(#288,93ac34cd, ubuntu 1.12 monolith and no other cell), whose own later head and merge intomainboth passed. I opened #316 with the six-run table and what I would do about it. It is a behaviour test with a real fix behind it, so it does not ride inside a deletion PR, and weakening it to get this one green is the thing I must not do.Where I would push back
The other obvious follow-up — a test pinning what
apply_azimuth_elevation!andapply_heading!do near the vertical, where the old frame-based placement was undefined — is #312. Also a behaviour test, also not this diff.Verification
grep -rn apply_heading, which found the definition, the header line and the docs entry and no call.test/test_transform.jl:Transform Tests | 325 325— all pass, twice (4m31s cold, 17.5s on the warm cache after the last edit). juliaserver,examplesenv.test/test_heading_calculation.jl:Heading - Circular Path | 72 72,Heading - Horizontal Circle | 48 48,Heading - Special Cases | 22 22. This is the file that keepscalc_R_t_to_whonest now that onlycalc_headingreaches it.test/test_twist_alignment.jl(the file CI errored in): green here, and bit-identical with the parent commit'stransforms.jlswapped back in — numbers above.agent ci-local): PASS (70 min, Julia 1.12.7, one cell of the matrix) — weak evidence next to GitHub CI, but it is the cell that went red there.Julia 1.11 - ubuntu,Julia 1.12 - windows,Julia 1.12 - macOSand the kernel-backend cell on the same ubuntu 1.12 image.Julia 1.12 - ubuntu monolithis the one red, ontest_twist_alignment.jl→ test_twist_alignment asserts on a state reached by driving the VSM to the edge of convergence #316, not this diff. I re-ran that cell alone and its six siblings went green a second time; the cell itself was still running when I wrote this, 1h28m into a 180-minute budget, so this PR's own check badge is the live answer and I am not transcribing a guess for it. Either verdict leaves the reading unchanged — green makes it the flake test_twist_alignment asserts on a state reached by driving the VSM to the edge of convergence #316 describes, red makes it the second sighting on this commit, and neither can come from a diff that is bit-identical on this test.docs/carries no manifest and building it would resolve one, whichAGENTS.md§2 keeps out of my hands. The@docsentry and the docstring it pointed at go in the same commit, so there is no orphan either way; the Documentation job is green.reuseis not installed on this box. Neither touched file changes a licence header, and thereuse-lintjob is green.main: the branch was atorigin/mainwhen pushed, no rebase needed.calc_R_t_to_wcalls per transform at init only, not on any stepped path.SymbolicAWEModels.apply_headingthrough the private API it breaks — it is not exported and not in the public docs.Scope
+8 / -30 across 2 files:
src/system_structure/transforms.jland one line offdocs/src/private_functions.md. No stack — no open PR touchestransforms.jl. Found while answering #254; filed #316 on the way.Closes #313 · task
SymbolicAWEModels.jl-313