Repository navigation
Conversation
BREAKING. Follow awesIO b5a9a60: the system file owns materials and whole-component tether quantities, the structure the discretisation. SystemDefinition holds both, load_structure checks system_sha on the file's bytes, system_document writes the union of several definitions, and to_yaml writes YAML 1.2 that 1.1 readers agree on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1-Bort-1
left a comment
There was a problem hiding this comment.
Independent review (advisory)
Verdict: REQUEST CHANGES · 4 inline, 0 off the diff
Good
- Hash refusal is right:
load_structurehashes the named file's bytes relative to the structure and refuses a mismatch; read insrc/document.jl:259-270, andtest-structure.jlcovers a one-byte change. - The union by name refuses one name with two values for both materials and tethers; read
union_by_name, and tests cover both refusals. - The writer quotes
-and writes1.0e-12and5.5e+10; readsrc/yaml_writer.jl, and the test matches every V3 float against PyYAML's float pattern. note,description,systemandsystem_shaare caller-settable on the writers, as the issue asks; readstructure_documentandsystem_document.- No existing test assertion was weakened; the test diff against #86's tip only adds the system argument, moves fixture paths and adds cases.
- The vendored
v3_system.ymlholds the 2 tethers and 6 materials the issue compares against, andsystem_document(load_structure(path))is tested equal to it for both fixtures. - Renaming a material as the card suggests is safe, since
Rowslooks names up by scanning and references are resolved to indices; readsrc/rows.jlanddocument_value. - Tests and the card's V3 file checks were not rerun for this review; the above comes from reading the code and tests.
Not good
src/authoring.jl:1208— Every authoring tether becomes a system tether, so the V3 system has 88 tethers and 146 materials and the beam tether's material isbridle_line, against the issue's 'that system lists both' and v3_system.yml as what the writer 'should reproduce in content' (2 tethers, 6 materials,v3_beam_line). The card hands this to the awesIO#39 review, but this unit owns it: 86 bridle lines would be published asnon_conductive_tether, and the vendored beam structure's single tether no longer matches.src/document.jl:135— Nothing checks that each structure tether names a system tether, or that its segments agree with that tether's length and diameter, though the vendored schema requires the name and the card says awesIO's validator does too. A file awesIO refuses loads here (narrowed_systemin the tests relies on it), which leaves open the disagreement between files that this PR exists to end.CHANGELOG.md:19— The issue says 'structure_documentwrites both, and the system file's hash into the structure', but it returns the structure only and the caller must write the system, hash it and pass the hash back. The card names this as its own decision, and the cost is that a caller can pair a structure with a stale or hand-typed hash, caught only at load.src/document.jl:30— An emptysystemorsystem_shais a legal default, soto_yaml(definition)silently writessystem: ""andsystem_sha: "", which fail the schema'sminLengthandpattern, andload_structurerefuses that file. The new test helperschema_documentinjects a path to get past validation, so the exported default path is left producing invalid files with no error.- The vendored examples are still the script-converted ones, so the issue's re-vendor from regenerated files and from the merge is still open before this can merge.
- No test runs
system_documenton the real beam and PSM pair fromload_authoring; the union tests usenarrowed_systemcopies of the beam fixture, so the issue's first use rests on the card's manual run. test/test-log.jl:10is 97 characters, over the 92-column limit of rubric §10.- The
SystemDefinition(document, system_document)docstring explains why with 'which has the file's bytes', which rubric §4 sends to the card. - The card does not say what was searched for before adding the YAML writer and helpers, as rubric §2 requires.
yaml_flowhas no method for a callable, soto_yamlof a definition with a callable-stiffness material throws a bare MethodError rather than a message naming the material.definition(log)on a log saved before this change fails with a bare KeyError onsystem, not a message saying the log predates the system key.
claude, rubric CLEAN_CODE.md. A different lab from the implementer
on purpose: a reviewer sharing its blind spots would not flag its mistakes.
| system.spanwise_stations, system.pulleys, system.tethers, | ||
| system.winches, system.wings, | ||
| system.canopy_faces, bodies, system.tubes, system.extras) | ||
| system_tethers = [design_tether(system, tether, segments) for tether in system.tethers] |
There was a problem hiding this comment.
MAJOR: Every authoring tether becomes a system tether, so the V3 system has 88 tethers and 146 materials and the beam tether's material is bridle_line, against the issue's 'that system lists both' and v3_system.yml as what the writer 'should reproduce in content' (2 tethers, 6 materials, v3_beam_line). The card hands this to the awesIO#39 review, but this unit owns it: 86 bridle lines would be published as non_conductive_tether, and the vendored beam structure's single tether no longer matches.
There was a problem hiding this comment.
Fixed: a system now lists only the tethers a winch holds, and a structure file only the tethers its system lists, so the V3 system has 2 tethers and the 86 bridle lines stay as segments. Not fixed: the 146 materials, since each canopy spring in V3Kite has its own modulus and the 6 came from a script's stand-in, and the name bridle_line, which comes from V3Kite's variable and a caller can rename. Both are on the card.
|
Local full suite: PASS (7 min, Julia 1.13.3, one cell of the matrix) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The regenerated examples carry SAM's columns, so the tests read them with a stand-in for SAM's default models, and fixture_document keeps the schema's columns for the tests about reading. The round trip now pins the writer's bytes against all three files. Tests follow the files: bridle_line for v3_beam_line, a beam tether length within rounding of 250 m, and the wing's quaternion compared with the file's. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A structure file used to hold its own materials and repeat what the system file says about the tether, so the two could disagree. awesIO now settles this: the system file owns the design and the structure owns the discretisation. This package now follows that. Loading a structure also reads the system file it names, and refuses it if the hash recorded in the structure does not match that file's bytes. One system can now serve several structures, which is what the V3 kite needs: its beam and particle models share one
v3_system.yml.Writing a system takes the union of the definitions' materials and tethers by name, and refuses one name with two different values. YAML now comes from a small writer of our own, because the old one left a unit of
-unquoted and wrote5.5e10, which PyYAML reads as a string.Decisions I made:
structure_documentreturns the structure only, andsystem_documentreturns the system. They are two files, and one system is written once for several structures, so a structure takes the path and hash of a system already on disk. A caller can pass a stale hash, whichload_structurethen refuses.load_authoring, so a definition read back from its files has one tether where the authored one has 87.to_yaml(definition)refuses a definition that names no system file, because that file could not be loaded.structure_documentandto_jsonstill allow it, for a log and for writing the system first.strict=falsedrops them. A silent drop would lose them on rewriting, and keeping unread columns would let a typo through. This package's tests read those files with a small stand-in for SymbolicAWEModels' models, so nothing here depends on it.test/data/structure/andtest/data/system_config/, so the relativesystempath they hold resolves unchanged.src/and KiteUtils for a writer and a file hash: the only one was YAML.jl'sYAML.write, which is what wrote the unquoted unit and5.5e10.The V3 files themselves come from this writer, through 1-Bart-1/awesIO#39, from V3Kite's own project files. Their system lists 2 tethers and 146 materials, of which 145 are linear, because each canopy spring in V3Kite has its own modulus. The beam tether's material is named
bridle_line, because it has the same values as that variable, and its length is 250.00000000000006 m, summed from its segments and within awesIO's tolerance.SymbolicAWEModels has to change these (its unit
samsystem), read at itsmain78601c5:src/definition_loader.jl:344passes a system toSystemDefinition, andsys_struct_from_document,save_structure_documentandload_structure_documentcarry both files and the hash.src/definition_loader.jl:326forwards the new keywords of itsstructure_documentmethod and gains asystem_documentone.src/system_structure/definition.jl:14gives the tethers its winches hold assystem_tethers, or neither file lists a tether.to_yaml(definition)on a definition with no system file now passes throughstructure_documentwith the path and hash.test/test_structure_document.jl:191,207,221read back with the system.[compat]bound moves to the next minor. Saving a log needs no change there, assave_loghere writes both.Stacked on #86, which merges first. Ready for review together with 1-Bart-1/awesIO#39, which merges before this. This vendors that branch at 1-Bart-1/awesIO@5dc983b, so it can't merge until it is vendored again from that merge. Each V3 file's note names f9a13c9, so this branch keeps that commit.
Breaking, so it goes out as the next minor.
Verification
load_structurethen the writer reproduces both structures and the system byte for byte, and a test now holds that. awesIO's own validator and its readers ran on these files in awesIO: the system file owns the design, and the examples are written by KiteGeometry 1-Bart-1/awesIO#39, not here.bridle_lineforv3_beam_line, the tether's length within rounding of 250 m, the wing's quaternion compared with the file's rather than with the identity, which it now misses by 3e-17).narrowed_systemnarrows a loaded definition, and two authoring tests comparestructure_document, becauseto_yamlof a definition with no system file is refused. The properties they protect are unchanged.Opened by
1-Bort-1, an AI agent working for @1-Bart-1.Closes #87 · task
KiteGeometry.jl-87