Repository navigation
Write KA orientations to SysState, keep KS inside the model - #312
Conversation
KiteUtils 0.13 defines SysState quaternions as KA (aft-right-up against ENU). The model is unchanged: kite_ref_frame and calc_orient_quat stay KS, and update_sys_state! converts at the boundary. - roll, pitch and yaw are unchanged; they stay KS, against NED - turn_rates is KA, so its z component has the opposite sign to before - calc_heading(s) passes the quaternion instead of Euler angles, one round trip through quat2euler fewer, same angle Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
KiteUtils 0.13 dropped the fields: they were the same orientation the quaternion already holds, and keeping them kept a second convention in the state. euler_ks(ss.orient) reports them, still against NED. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The WinchModels and AtmosphericModels releases that accept KiteUtils 0.13 (both 0.3.11) require Julia 1.12, so KiteModels on KiteUtils 0.13 cannot support 1.11. Removes its manifest, CI cell, install branch and docs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- fromKS2KA / fromKS2KA_body / fromENU2NED replace the names KiteUtils 0.13 did not ship; calc_heading(s) hands it a KA attitude. - examples_3d and test take KiteViewers from OpenSourceAWE/KiteViewers.jl#56's branch until a release accepts KiteUtils 0.13. - Minimal resolve on 1.12 and 1.13: KiteUtils 0.12.2 -> 0.13.1, WinchModels 0.3.10 -> 0.3.11, nothing else. - Tests assert the KA orientation, euler_KS round trip and KA turn rate; the parking examples stop overwriting sys_state.orient with KS and plot the KS body rate next to heading_rate. - test-kps3: heading at azimuth 0 is checked modulo 2π; ≈ 0 had no absolute tolerance and failed on 2.8e-17. - calc_orient_quat is on the functions page. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1-Bort-1
left a comment
There was a problem hiding this comment.
Independent review (advisory)
Verdict: REQUEST CHANGES · 3 inline, 0 off the diff
Good
- The frame change stays at the boundary:
update_sys_state!wrapscalc_orient_quatinfromKS2KA, andkite_ref_frameandcalc_orient_quatare unchanged (read in src/KiteModels.jl). something(x, NaN)replaces threeisnothingif/else blocks, so the writes toset_force,set_torqueandset_speedbecome one line each without changing behaviour.- test-update-sys-state.jl drops the padded
[x, 0, 0, 0]expectations and checksturn_ratesandorientthrough the inverse conversions, so each assertion still tests a real value. calc_orient_quatgets a docstring and an entry in docs/src/functions.md, so it can be reached from the built docs.
Not good
CHANGELOG.md:7— This entry says 'Requires KiteUtils 0.12', but the compat bound is now 0.13, and the entry sits under v0.11.17, which is already dated 2026-08-12. changelog.d/frame-unification.md also describes the same bump, so the requirement is stated twice and the two places disagree..github/workflows/Test.yml:19— Dropping Julia 1.11 from CI, the compat bound, the docs and the manifests is a separate breaking support change riding in a frame-conversion PR, and the card does not mention it at all (§1). The card needs to name it, or it belongs in its own PR.CONTRIBUTING.md:107— This line claims the package supports 1.13 on Linux, but CI's matrix is '1.12' and '1' and the stdlib compat entries stop at 1.12, so nothing here tests 1.13.- The card no longer matches the branch. It says the 0.12 SysState migration is 'not attempted here' and the change is 'three edits', but the branch merges #321, rewrites bin/install, deletes bin/setup_env and bin/jetls_with_env, relinks README and docs to OpenSourceAWE, and touches 30 files.
- The card says roll, pitch and yaw are 'unchanged'. changelog.d/frame-unification.md says the opposite ('BREAKING: … no longer written to SysState'), and the diff does delete the
ss.roll, ss.pitch, ss.yaw = orient_euler(s)line. The change downstream consumers will notice most is missing from the card. - test/Project.toml adds a
[sources]KiteViewers entry pinned to the unmerged branchagent/54-update-kiteutils, and the card does not mention it (§9). The test environment then depends on a branch that will be deleted. - Project.toml raises
juliato "1.12, 1.13" but leaves the stdlib compat at LinearAlgebra, Logging and Pkg "1.11, 1.12" and REPL "1.11.0, 1.12.0". A 1.13 install will likely fail to resolve, and 1.11 is now a dead entry. - The inline comment above
ss.orient .= fromKS2KA(...)repeats what the newcalc_orient_quatdocstring already says. It is a 'why' comment and fails the deletion test. - The
calc_orient_quatdocstring namesSysStateandupdate_sys_state!, which is rationale about its callers rather than what the function returns. - README 'Installation as package' now ends with trailing whitespace, and docs/src/index.md keeps 'make sure that Python3 and Matplotlib are installed:' but deletes the command that followed the colon.
- The 'See also' sections in README and docs/src/index.md still link KitePodModels to aenarete, although the same PR moves every other link to OpenSourceAWE.
- The docs/src/index.md subcomponent list silently drops the VortexStepMethod / V3Kite / RamAirKites bullet, and neither the card nor the changelog mentions it.
- The card says the test suite has not been run, and none of the checklist boxes are ticked, so nothing verifies the
turn_ratessign flip or the heading equivalence against a live model.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| matrix: | ||
| version: | ||
| - '1.11' | ||
| - '1.12' |
There was a problem hiding this comment.
MAJOR: Dropping Julia 1.11 from CI, the compat bound, the docs and the manifests is a separate breaking support change riding in a frame-conversion PR, and the card does not mention it at all (§1). The card needs to name it, or it belongs in its own PR.
There was a problem hiding this comment.
The 1.11 drop was agreed on this thread (option A) — this branch can't build without it, since WinchModels/AtmosphericModels 0.3.11 need 1.12. Now named in the PR body.
|
Not a bug in this PR: the one error is #314, the same failure that happens on |
Resolve conflicts: keep main's CHANGELOG.md (the KiteUtils 0.13 changes are in changelog.d/frame-unification.md), WinchModels 0.3.12 compat, the docs wording for Julia 1.12 or 1.13, and main's default manifests with KiteUtils 0.13.1 and KiteViewers from the agent/54-update-kiteutils branch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…G.md Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
TL;DR
SysStatenow carriesKAorientations (aft-right-up, against ENU), the convention KiteUtils 0.13 stores; the model keeps working inKSandupdate_sys_state!converts at the boundary. This is released as KiteModels v0.12.0 (breaking): version, compat andCHANGELOG.mdare already set. The KiteUtils 0.12SysStatemigration (#321) and dropping Julia 1.11 are already onmain(v0.11.19, v0.11.20), so they are no longer part of this diff.What a downstream consumer will notice
roll,pitch,yaware no longer written toSysState— KiteUtils 0.13 removed those fields.orient_euler(s)still returns them against NED, andeuler_KS(ss.orient)recovers them from a state or a log.ss.orientisKA, notKS.calc_orient_quatandkite_ref_framestill returnKS.turn_ratesisKA, so its z component has the opposite sign to before (a rate about an axis that pointed down and now points up).calc_heading(s)passes the quaternion instead of Euler angles; same angle, onequat2eulerround trip fewer.Project.toml,docs/Project.toml); the default manifests for Julia 1.12 and 1.13 use KiteUtils 0.13.1.KiteViewers from a branch, until KiteViewers is released
No released KiteViewers accepts KiteUtils 0.13 yet.
examples_3d/Project.tomlandtest/Project.tomltake it via[sources]from OpenSourceAWE/KiteViewers.jl#56's branch (agent/54-update-kiteutils), and the default manifests record that branch.This PR merges before KiteViewers is released, to break a circular dependency: KiteViewers has
KiteModelsin itstesttarget, so OpenSourceAWE/KiteViewers.jl#56's CI needs a registered KiteModels that accepts KiteUtils 0.13. Registration only reads KiteModels' rootProject.toml, which does not depend on KiteViewers, so the[sources]lines do not stop the release. The order is:KiteModels = "0.12", setjulia = "1.12, 1.13", refresh its default manifests; CI can then resolve.nedKiteViewers.jl#50 and Add abstract vehicle interface #56, release KiteViewers v0.7.0.[sources]lines, set the KiteViewers compat inexamples_3d/Project.tomlto the release, move the default manifests to the registered KiteViewers (needed anyway for the "released versions only" check inbin/update_default_manifest).Also in the diff
test-kps3.jl:calc_heading(kps) ≈ 0had no absolute tolerance; the quaternion route gives 2.8e-17, the Euler route 2π − ε. It now compares modulo 2π.examples_3d/parking_*.jl: stop overwritingsys_state.orientwith aKSquaternion, and plot theKSbody rate next toheading_rateso the curves keep the same sign.examples_3d/Project.toml:KiteModels = "0.12".calc_orient_quatgets a docstring and a place indocs/src/functions.md.changelog.d/frame-unification.mdmoved into thev0.12.0section ofCHANGELOG.md.Review order:
src/KiteModels.jl, thentest/test-update-sys-state.jl.Verification
test/test-update-sys-state.jlfails without the KA conversion (orient,turn_rates), passes with it.mainat v0.11.20):Testing KiteModels732/732.Pkg.resolve()of the workspace succeeds on Julia 1.12 and 1.13 with the default manifests.examples_3dparking examples not run (need a display).[sources]lines pin a branch that will be deleted after KiteViewers#56 merges; step 4 above must land before that.Scope
13 files, +59/−32. Source change is
src/KiteModels.jl(+12/−8).Followed by: OpenSourceAWE/KiteViewers.jl#56 (needs the v0.12.0 release of this PR)
🤖 Generated with Claude Code