Repository navigation
91: AR50 legend splits forest by tree type and open firm ground by vegetation cover - #339
Merged
Merged
Conversation
…its open firm ground by vegetation cover (@architect) docs/increments/91-ar50-vegetation-cover.md and its ROADMAP row. - Format: one optional [split] table in a coded legend (class code, second attribute, [[split.class]] value/code/label/colour); five loader rules; sub-classes inherit the parent's edges; an unlisted second value keeps the parent code; a first-attribute value equal to a sub-class code is refused. - ar50: 50 split by vegetasjonsdekke into 5051, 5052, 5054, 5055, 5099 (100 x 50 + value; arealtype codes are two digits), NIBIO's ARVEGET colours; 99 is its own sub-class (NIBIO colours it apart); 53 left out (in none of the 15 county files, measured). - Readers gain an optional second column (GeoPackage, GML 2, GeoJSON); land_cover_codes text names the split; rasputin sources lists the split attribute. Writers, palette and MCP descriptions unchanged. - Measured on a 15 km Jotunheimen box, DTM10, tolerance 5: land-cover vertices 10,638 -> 25,550, triangles 143,549 -> 156,671, 2.35 -> 2.69 s. - About 60 counted lines, full path, no C++, no @Perf. Eight red tests named. - Found: 1,716 of Innlandet's AR50 polygons are invalid; a mesh over Bygdin stops with a raw GEOS TopologyException (handed back as an idea). Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ne rule for a missing second attribute (@architect) After @Reviewer's design review round 1 (CHANGES REQUESTED) and Ola's ruling to split forest as well: - Rows from read_source stay (fid, geometry, value) 3-tuples; the extra attributes come in a parallel SourceRows.extra. Every caller is listed from git grep (_Tally.source, read_lake_polygons, tolerance_field shapes_in, network_file units_at); only _Tally reads extra, the others and their tests are unchanged. - One rule for all three readers: a missing second attribute (GeoPackage column, GML element, GeoJSON property) is no value, so the parent's code stays; nothing is refused, so AR50 exports without the column still mesh. - Values match by a key: whole numbers as integers (52, 52.0, "52", "52.0" alike), other text as itself, bools, NaN and fractions no key. - The rasputin sources listing change and its test are dropped. - A legend carries a list of [[split]] tables; ar50 splits 30 by treslag (3031, 3032, 3033, 3099; NIBIO's colours, 3099 forest's own) and 50 by vegetasjonsdekke. Ola's reason recorded; the comparison of the two forest approaches is a later step. - Cost measured (median of three after a warm-up): Numedalslagen with the quick check's flags and AR50 in place of CORINE, 1,587,559 to 2,178,870 triangles, 80.1 s to 91.8 s; Jotunheimen +9 % triangles; a forested box at Elverum +6 %. The CORINE quick-check cases expect no cost. - The Numedalslagen probe needed repaired copies: ar50 over the downloaded county files stops today with a GEOS TopologyException (1,229 invalid polygons, and 54 more invalid after the move to EPSG:25833). - Size about 70 counted lines. Co-Authored-By: Claude Opus 5.5 <[email protected]>
… applied (@architect) Applies @Reviewer's two wordings from design review round 2 at 4297480: - the invalid-polygon note in the increment file now says the crash is a separate fix ruled by Ola 2026-10-22, that this increment's code does not depend on it, and that section 5's Numedalslagen figures come from repaired county files; - the increment's ROADMAP row says the same for its Numedalslagen figures. Adds a ## Review section with the round 2 record as one line. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…rest and open firm ground (@tester) The suite of docs/increments/91-ar50-vegetation-cover.md, section 8, tests 1 to 7. 47 tests red at this commit, for the missing feature (no `[[split]]` in the loader, no `Legend.splits`, no `feature_input.split_key`, no `extra=` on `read_source` or `read_gml`, no sub-class codes on triangles). Green at red, by design: a split in an uncoded legend is refused (today as an unknown key), sub-class masks are land_cover, and the mesh without the second attributes keeps 30 and 50. Amended: TestAr50 palette (19 entries) and test_12 (2 * 19 annotations). Pinned beyond the design, for @architect: - Each loader refusal also names the offending code, value or key (40, 30, COVER, water, edges, 51, 10, 5051, 3031; "split" for an uncoded legend). - The full land_cover_codes text: "AR50 arealtype code, attribute arealtype, <clauses>, legend ar50; ..." (the clauses joined by ", "). - `Split.codes` read as a mapping; `Legend.splits == ()` with no split. - `read_gml` features without `extra` have `extra == ()`. - A value with letters keys as itself in the loader too (`"5a"`). No C++ change; no build needed. No mutation round (none named). Co-Authored-By: Claude Opus 5.5 <[email protected]>
…chitect) Section 8 gains "Pinned by the red suite": each split refusal names what is wrong (and the joined "listed twice" check now names the code); the whole land_cover_codes text with the split clauses joined by ", "; splits == () without a split and Split.codes any Mapping; GmlFeature.extra == () without extra; the loader keys a lettered value as itself. Fixture A's figures rechecked at b574d4a with gml_to_geopackage and sqlite3 typeof. No test changes asked. The tester's reading of the scratch-copy fix-run rule (meant for a fix named at a code location, not a feature) is accepted for this increment; the older tests this change could turn red were checked by grep (ar50 palette and annotations, already amended; no test reads "listed twice", the ar50 land_cover_codes text, or constructs GmlFeature or RawFeature by position). Co-Authored-By: Claude Opus 5.5 <[email protected]>
…forest and open firm ground (@developer) The loader reads optional [[split]] tables (rules 1 to 5 of docs/increments/91-ar50-vegetation-cover.md, section 3), adds each sub-class to the palette and gives Legend.splits sorted by class code. feature_input gains Split and split_key (one key function for the loader and a feature's value); read_source, query_features and read_gml read the extra attributes asked for (None where absent, () when none is asked), and _code turns a split class into its sub-class code, refusing a first value that equals a sub-class code. land_cover_codes names each split. ar50.toml gains the forest (treslag) and open-firm-ground (vegetasjonsdekke) splits. Rows stay 3-tuples; no test file touched. Full Python suite green (9612 passed), mypy, ruff check and ruff format --check clean; governance gates green. No C++ change, so no C++ build or sanitizer run. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tester) Code review round 1 at 0efd87f, blocking bug: `_Tally.source` lowercases the split attribute names before passing them to the readers, and the GeoJSON and GML readers match names exactly, so a legend splitting by `Veg` splits nothing in those formats. New class TestASplitAttributeAsWritten in tests/python/test_feature_input.py: a TOML legend (coded, splits 50 by `Veg` and 30 by `tre`) loaded through `legends.load`, then `open_features` on a GeoJSON file and on a GML file. Both red at this commit: the `Veg` feature gets 50, expected 5051; the lowercase `tre` feature gets 3031 in both, as before. Checked against the fix the reviewer suggested, applied to a scratch copy (duplicates removed case-insensitively, first spelling passed as written, per-row values keyed by the lowercase name): the whole Python suite passes there, 9595 passed, 37 skipped, 1 xfailed; no older test turns red. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…t (@developer) Code review round 1 at 0efd87f found that `_Tally.source` lowercased the split attribute names before passing them to the readers; the GML and GeoJSON readers match names exactly, so a legend splitting by `Veg` split nothing in those formats. Now repeated names are dropped regardless of case, the first spelling in the legend is passed to the reader as written, and each row's second values stay keyed by the lowercase name, which `_code` looks up. Makes TestASplitAttributeAsWritten (tests/python/test_feature_input.py) green. No test file touched. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…rings in #335, #337, #338 (@developer) Merge base 2600a74. First-parent merges brought in (git log --merges --first-parent --oneline 2600a74..acef220): acef220 Merge pull request #338 from expertanalytics/worktree-retro-1022c ffbae90 Merge pull request #337 from expertanalytics/worktree-worktree-mcp-reload-sources c22fd79 Merge pull request #335 from expertanalytics/worktree-worktree-rule-cuts-1022 Conflicts resolved by hand: ROADMAP.md Gates on the merged tree, in /Users/skavhaug/projects/rasputin/.claude/worktrees/worktree-ar50-vegetation/.venv: check_prohibited_deps, check_detria_boundary, check_citations --base origin/master, ruff check, ruff format --check, mypy, pytest. _core rebuilt: no, no C++ came in. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…e splits do; code review round 1 recorded (@architect) Code review round 1 at 0efd87f found that the prose edits listed in section 7 of docs/increments/91-ar50-vegetation-cover.md were never made. They now match the code as built (green 0efd87f, case fix 32a36b4, master merged in 4fa7a9e): - README.md, "Features as constraints": the ar50 legend splits forest by treslag (3031 to 3099) and open firm ground by vegetasjonsdekke (5051 to 5099). - docs/GLOSSARY.md, Legend: a coded legend may split classes by a second attribute into sub-classes with codes of their own; an unlisted value keeps the parent's code. - project_structure.md: the legends.py row names the [[split]] tables; the data/legends/ row says ar50 splits 30 and 50. - ROADMAP.md row 91 and the increment's status line: built, in review. Review section: one line for code review round 1 (CHANGES REQUESTED at 0efd87f, range 2600a74..0efd87f, 90 counted lines, two blocking, fixed in 614a613, 32a36b4 and this commit). Docs only, 0 production lines; check_citations.py exits 0. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
skavhaug
enabled auto-merge
October 23, 2026 08:21
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Oct 23, 2026
Post-merge check of #339; 91 marked merged
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.
A land-cover legend can split a class by a second attribute. AR50 splits forest (arealtype 30) by tree type (treslag) and open firm ground (arealtype 50) by vegetation cover (vegetasjonsdekke).
Design, red, green and review are in docs/increments/91-ar50-vegetation-cover.md. Code review round 2 APPROVED at dac60f4; 93 counted lines; full Python suite 9624 passed. Ola's push and merge yes 2026-10-23.
🤖 Generated with Claude Code