Repository navigation
Conversation
KiteUtils 0.13 defines `quat2viewer(attitude)` and nothing else, so the two-argument call did not just mis-draw a KS log: the `@compile_workload` calls `update_system`, and the package failed to precompile. Also drops the commented-out three-line kite enlargement from the same function. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8gC66GuqKbtyojKrcQP9M
It still allowed 0.11.13 and 0.12, against the 0.13 the root project asks for, so the workspace could not resolve even in principle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8gC66GuqKbtyojKrcQP9M
|
Local full suite: FAIL (0 min, Julia 1.13.0, one cell of the matrix) |
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 two-line fix matches the released KiteUtils 0.13 API —
src/common.jl:373-374now converts withfromKS2KAand calls the single-argumentquat2viewer, which is what the precompile workload (src/KiteViewers.jl:139, defaultframe=KA) hits, sousing KiteViewerscan precompile again. - The new test pins the actual rendered value: it reads
KiteViewers.quat[], the quaternion the kite mesh is rotated by, not an intermediate, so it goes red on a missing conversion. - Scope is exactly the stated idea (+17/-9, four files); the only extra is deleting a commented-out three-line-kite block inside the same function, which §6 asks for and the card names.
examples/Project.tomlfollowing the root project toKiteUtils = "0.13"is consistent — leaving it at0.11.13, 0.12inside one workspace would be unresolvable either way.- No CHANGELOG entry is the right call and is argued: #50's
Unreleasedentry already states the behaviour, and a line saying the previous commit did not precompile is about the branch, not the release.
Not good
test/runtests.jl:8—test_frames.jlcreates aViewer3Dat top level andtest_parking.jlthen creates a second one in the same session, which CLAUDE.md calls unsupported — the two sharequat,textnodeand the rest of the module globals. The assertions survive only because test_frames reads its quaternion before test_parking starts; a later reorder or a second frame test would break silently.- The test compares the KS path against the KA path; if the KA↔KS transform is an involution (as the card implies) it would also pass with
fromKA2KSinsrc/common.jl:373— numerically equivalent, but the test does not pin the name. frame == KS ? ... : state.orienttreats every non-KSmember ofFrameConventionasKAsilently; harmless if the enum has exactly two members, worth a thought if it ever grows.- One attitude from
demo_stateis the whole coverage forframe=KS— flagged in the card, and acceptable while nothing else resolves. - Neither the new test nor the suite can actually run until KiteModels ships its 0.13 support, so this lands unverified by CI; the card is explicit about it and the PR stays draft.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| using Test | ||
|
|
||
| cd("..") | ||
| include("test_frames.jl") |
There was a problem hiding this comment.
MINOR: test_frames.jl creates a Viewer3D at top level and test_parking.jl then creates a second one in the same session, which CLAUDE.md calls unsupported — the two share quat, textnode and the rest of the module globals. The assertions survive only because test_frames reads its quaternion before test_parking starts; a later reorder or a second frame test would break silently.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3NVPfaDhK4ASic8HuBvgx
|
CI: https://github.com/OpenSourceAWE/KiteViewers.jl/actions/runs/34886316033/job/104117740896 |
- 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>
Depends-On: OpenSourceAWE/KiteModels.jl#312
Depends-On: OpenSourceAWE/KiteModels.jl#321
Depends-On: aenarete/WinchModels.jl#26
TL;DR
#50 was written before KiteUtils 0.13 was released and calls a two-argument
quat2viewerthat the release does not define, which stops KiteViewers precompiling at all; the caller now converts aKSattitude itself withfromKS2KA.examples/Project.tomlfollows the root project toKiteUtils = "0.13".The call that is not there
KiteUtils v0.13.0 was released today with
quat2viewer(attitude)and no second method —src/transformations.jl:140. AKSorientation is the caller's job to convert, which is what the release notes of OpenSourceAWE/KiteUtils.jl#130 say:quat2viewer(fromKS2KA(q)).This is not a wrong-looking kite on an old log.
update_systemis called from the@compile_workloadinsrc/KiteViewers.jl:139, so the method that does not exist is reached while the package is being precompiled, andusing KiteViewersfails outright:test/test_frames.jlpins what #50 promises and what this restores — that aKSattitude and theKAattitude it converts to are drawn the same way round. It reads backKiteViewers.quat[], the quaternion the kite mesh is actually rotated by, so it fails on the wrong conversion as well as on no conversion, and it does so over five attitudes rather than the demo state's one: the demo attitude, the identity, two with all four components at 0.5, and one more about a single axis. Each is written intostate.orientasKA, drawn, then written again as itsfromKA2KSimage and drawn withframe=KS. Deleting the conversion insrc/common.jlturns all five red; restoring it turns all five green.atol=1e-6rather than the default relative tolerance, which components sitting at zero can never meet.What stops this going green, and what is being done about it
The registered packages give this branch no environment it can install:
ERROR: empty intersection between KiteUtils@0.11.13 and project compatibility 0.13. That is also why the red check stops inPkg.test()'s resolve, before any test file. It is neither a code bug nor a test bug. Two of the four packages that held KiteUtils down have since released with 0.13 (AtmosphericModels v0.3.11, KitePodModels v0.4.2). The other two are fixed from this task:KiteUtils = "0.10, 0.11, 0.12, 0.13". It reads only winchSettingsfields; its tests pass against 0.13.0 on Julia 1.12.7.SysStatemigration that Write KA orientations to SysState, keep KS inside the model KiteModels.jl#312 says it is blocked on (theSysState(P)constructor, one-slot winch fields).test-update-sys-state.jlgoes fromMethodError: no method matching (SysState{11})()to 58/58. #312 then goes on top of it for 0.13, and four of its calls need the names KiteUtils 0.13.0 actually shipped with (fromKS2KA,fromKS2KA_body,calc_headingon a KA attitude,fromENU2NED); that card lists them line by line.With both upstream branches and a local merge of #312 carrying those four renames, this branch's whole suite runs green against KiteUtils 0.13.0:
test_frames.jl5/5, thentest_parking.jl(the 20 s KPS4 run) with its elevation, azimuth and winch-force assertions 3/3. The overrides lived only in the gitignored live manifest, so the tracked.defaults still pin KiteUtils 0.11.13. They move by the minimal resolve once WinchModels and KiteModels release, and until then this stays draft.Found on the way, not fixed here
steering_4p.jl,depower_simple.jl,joystick.jl,reelout_*.jl, the*_bench_video.jlpair) andtest_parking.jl/test_steering.jlall callupdate_systemwith the default frame. That is correct after KiteModels.jl#312, and wrong before it — but they cannot run before it either, so there is nothing to write differently. Worth re-reading once that lands..github/workflows/CI.ymlcarriesfail-fast: false. One matrix cell today, so it costs nothing yet.CHANGELOG.mdentry: Take the orientation convention as a keyword, notned#50'sUnreleasedentry already describes the behaviour this makes true, and a line saying the previous commit did not work is a line about the branch, not about the release.bin/installhad drifted badly enough that./bin/install -yexits 1 on a menu nothing can answer, which is why this worktree arrived uninstalled. Rewritten in cleanup: bin/install takes -y, -u and -h and touches nothing outside the repo #55, offmain, rather than here: it touches no code this branch does. cleanup: bin/install takes -y, -u and -h and touches nothing outside the repo #55 also ignoresLocalPreferences.toml— every checkout here is expected to carry one and this repo alone did not ignore it, so it stands as an uncommitted change in every worktree. Both go through cleanup: bin/install takes -y, -u and -h and touches nothing outside the repo #55 rather than this branch because cleanup: bin/install takes -y, -u and -h and touches nothing outside the repo #55 can merge now and this cannot.Verification
Failed to precompile KiteViewers,MethodError: no method matching quat2viewer(::FrameQuat, ::FrameConvention)— full output abovetest/test_frames.jlred before, green after, in a KiteUtils 0.13.0 environment (juliaserver,5 passed;0 passed, 5 failedwith the conversion removed)test/runtests.jlon Julia 1.12.7,examplesenv, KiteModels = this migration + #312 + the four renames, WinchModels = the fork branch; local overrides, not what CI installs)update_systemis already ondocs/src/reference.mdagent ci-local/ GitHub CI: red at the resolve (Unsatisfiable requirements detected for package KiteUtils, run 34886316033), as above, until Accept KiteUtils 0.13, support Julia 1.12/1.13 only, test every line of src/ aenarete/WinchModels.jl#26 and Build SysState with the KiteUtils 0.12 constructor and one-slot winch fields KiteModels.jl#321 are releasedtest_parking.jlnow drives the viewer from a real KPS4 run writing KA, but asserts position and force, not the drawn orientation; that the kite is drawn the right way round is proven only for the five hand-written attitudes intest_frames.jl.Scope
+22 / -9 across four files, of which
src/common.jlis two lines of conversion and the deletion of a commented-out three-line-kite block in the same function. Stacked on #50;test/test_frames.jlis new.Opened elsewhere
Opened by
1-Bort-1, an AI agent working for @1-Bart-1.Closes #54 · task
KiteViewers.jl-54