0.4.0-0.4.2: network properties, correctness fixes, delivery hygiene - #3
Open
mikejohnson51 wants to merge 33 commits into
Open
0.4.0-0.4.2: network properties, correctness fixes, delivery hygiene#3mikejohnson51 wants to merge 33 commits into
mikejohnson51 wants to merge 33 commits into
Conversation
added 30 commits
June 26, 2026 22:06
Native replacement for hydroloom::add_levelpaths matching the get_streamorder/ get_hydroseq style: mainstem follows the largest-weight upstream branch, level path id = outlet hydrosequence. Integer-indexed topo loop (O(N)).
get_streamorder kept its accumulator as a CHARACTER-named vector and did so[nd] / so[contribs] / up[[nd]] lookups by name inside the topological loop -- each an O(n) scan, so the whole pass was O(n^2). It stalled mega-basins: ~44 min on the Mississippi's 461k reaches (and infeasible at Amazon scale). This is what actually hung the NA reference rebuild (not the splitter). Rewrite to the same integer-indexed pattern get_hydroseq/get_levelpath already use: map ids to positions once, integer-indexed children + topo order, accumulate into an UNNAMED positional vector. Identical Strahler output (existing tests unchanged); 461k now 1.5s vs ~44min. Added a 120k scale-guard test.
- Vendor gpkg metadata helpers (gpkg_exec / update_col / update_geom), topology helpers (hf_network_is_dag, hf_assert_network_dag, hf_recompute_hydroseq, hf_break_cycles) and hf_fmt_id from the build package; drop dead network_is_dag - Add topology + fmt-id tests - README: portable example (drop hardcoded local path) - Add THIRD_PARTY_NOTICES; tidy invariant comments
- get_pathlength(): distance along the network from each reach's outlet to the terminal outlet (NHDPlus PathLength). - get_streamlevel(): stream level over the level-path graph (NHDPlus StreamLeve). - get_pfafstetter(): hierarchical Pfafstetter basin codes, verified bit-for-bit against a 746-reach reference basin fixture. - Extend the network-properties vignette to the full get_* family with a runnable worked example; add the family to the pkgdown reference index (plus previously unindexed exports); cross-link the seven functions via @family. - Reword get_streamorder()/get_levelpath() docs to describe behavior directly. - Rename the read_hydrofabric() verbosity env var to hfutils_verbose. - Version 0.4.0 + NEWS.
Adds upstream_index(x, id, toid) to the network-properties family. A depth-first pre-order labelling (upstream_id) plus the strict upstream count (num_upstreams) encode the nested set model, so "everything upstream of X" becomes an O(1) integer range filter (upstream_id in (u, u + k]) with no traversal at query time. Reuses accumulate_downstream for num_upstreams (inheriting its acyclic check), expands the largest-upstream branch first so main stems stay contiguous, and flags divergences (duplicate ids) and cycles rather than producing a misleading index. Sibling of accumulate_downstream and get_streamorder; consumed by hydrofabric::add_upstream_index().
Groups a network into contiguous same-order runs by walking upstream_index pre-order, breaking each run at a confluence/branch boundary or (optionally) where stream order changes. Each group is a contiguous upstream_id range, so a size-budgeted tiler or partitioner can merge groups into balanced chunks that are always complete sub-networks. Sibling of upstream_index/get_streamorder.
Adds a TRADEMARKS.md policy and NOTICE trademark section (Apache-2.0 grants no mark rights; "hfutils" used descriptively), sets Mike Johnson as sole copyright holder with Justin Singh-Mohudpur as contributor, and bumps to 0.4.1 for the upstream_index / merge_groups additions.
Convert em-dashes to commas/colons/semicolons and unicode arrows to ASCII in NEWS, vignettes, CONTRIBUTING, THIRD_PARTY_NOTICES, and TRADEMARKS, and generalize the O(n^2) note in get_streamorder. Docs/comment-only.
Adds hf_upstream_index(flowpaths, nexus): the schema-aware wrapper that resolves the flowpath -> nexus -> flowpath topology and computes the nested-set index via upstream_index(). write_hydrofabric now stamps upstream_id / num_upstreams onto every flowpath-keyed layer when the list is a complete hydrofabric (attribute-only, schema-detected, graceful skip), so any gpkg written through it (per-VPU, merged, or subset) carries a correct-scope O(1) upstream index.
Two defects, both in the geometry round-trip. It routed input through terra::makeValid() |> terra::aggregate(). That perturbed shared boundaries enough that neighbouring groups came out overlapping a tiling input. Grouped sf::st_union() over already-valid geometry preserves the input tiling exactly. Input is now repaired only where sf::st_is_valid() reports a problem, since perturbing valid geometry was the whole source of the drift. It also cast the result to POLYGON and kept only the largest part per group, discarding the rest with a warning. A group whose members are genuinely disjoint is a multipart catchment, not an error, and dropping the smaller parts silently deleted ground. Output is MULTIPOLYGON, which is lossless. This path was latent rather than active in current data, but it is the same defect that had to be fixed independently in two downstream callers. Measured on 61,061 real divides in 23,910 groups: overlap introduced 20 km2 -> 0.000000 km2, area delta -0.0005 km2, and 21s rather than ~65s. Faster as well as exact -- the terra round-trip was buying nothing. One existing test asserted the discard behaviour, including its warning and all-POLYGON output. It encoded the bug, so the assertion is deliberately inverted to require conservation, and a second test now requires that a tiling input dissolves without creating overlap. Note this does NOT by itself reduce the aggregate-divide overlap observed downstream: measurement shows that overlap is introduced after the union, by a later stage-2 step, which is being localised separately.
Unanchored, ignore_lyrs dropped any real layer whose name merely contained gpkg_, rtree_ or sqlite_ (flowpaths_gpkg_v2), silently and with no error. Anchor the prefixes. Also ignore layer_styles and qgis_projects, which QGIS writes into a GeoPackage when a style or project is saved to it. Counted as user layers they made a styled single-layer fabric look ambiguous. Collapse the three copies of the default into one constant, and update the multi-layer test, which still asserted pre-d0a35ae behaviour.
lint.yaml runs with LINTR_ERROR_ON_LINT=true. Wrap two over-long SQL strings, and fix compound semicolons and spacing in the tests.
Downstreams resolved only through the nexus map, so with the default nexus = NULL the map was empty, every flowpath became a terminal, and the index came back degenerate: each node isolated, num_upstreams 0, no warning. write_hydrofabric passes nex = NULL whenever the written list has no nexus layer, so it stamped those values into the GeoPackage. A toid that is already a flowpath id is now a direct link; the rest still go through the nexus map. Tests cover nexus-less, nexus-hop and mixed networks, and check the nexus-less result against upstream_index over the same topology. Also clears this file's semicolon lints and spells modelled US-style.
read_sf_dataset passed a bare external vector to select(), deprecated since tidyselect 1.1.0; wrap it in all_of(). Guard the second on.exit disconnect in write_hydrofabric with suppressWarnings, matching the one above it. Drop %||%, which had no call sites and shadowed base R 4.4's version with different semantics. Document the upstream-index stamping in write_hydrofabric, a user-visible side effect the help page never mentioned.
Copyright read three ways: Mike Johnson in NOTICE, Mike Johnson and Lynker Spatial in the LICENSE appendix and THIRD_PARTY_NOTICES, and Mike Johnson as sole cph in DESCRIPTION. Settle on Lynker Spatial as cph and fnd; Mike Johnson stays aut, cre and maintainer.
check_pkgdown() failed on upstream_index, hf_upstream_index and merge_groups missing from the reference index, which fails the site build. The last green Website run predates those functions, so the break was queued for the next push to main. Also links Lynker Spatial on the authors page and bumps a stale actions/checkout@v3.
The shipped QGIS project misspelled the brand TRADEMARKS.md exists to protect. The .qgs member inside the archive still does, and will clear when the project is next re-exported. TRADEMARKS.md was the only top-level policy file missing from .Rbuildignore, so R CMD check flagged it.
The README opened with as_ogr(gpkg) on a fabric it describes as holding two layers, which errors as ambiguous. The dependency badge read 16/49 against 18 direct and 72 recursive. The network vignette said "both functions" while describing seven and accumulated a column its example frame lacks. add_measures documented neither the columns it adds nor its join key, though the wrong key silently zeroes areasqkm. upstream_index, hf_upstream_index and merge_groups appeared in no narrative docs and no NEWS entry; both vignettes now cover them. CONTRIBUTING documents the lint, spelling and pkgdown gates. spelling.R ran with error = FALSE, so 28 unrecognised words had piled up unnoticed; enable it and fill inst/WORDLIST. Regenerates NAMESPACE and the Rd files left stale by 9d2e4f7.
Writing the changed features to a temporary layer makes GDAL build a spatial index beside it: four rtree_* shadow tables, their triggers, and a gpkg_extensions row. Cleanup removed only the feature table and its gpkg_contents and gpkg_geometry_columns rows, so the rest stayed in the file and accumulated per call. Ten updates left forty orphan tables. Removal now goes through .gpkg_drop_layer inside the same transaction. Verified over ten updates: no orphans, trigger count unchanged. Name the temp layer from tempfile() rather than the clock, which collided within a centisecond and put a "." in a SQL identifier. Adds the first tests for the three destructive mutation entry points. Writing them surfaced a requirement now in gpkg_exec's help: an UPDATE on a layer with geometry needs drop_triggers_for even when it touches only attributes, because the R-tree triggers call ST_IsEmpty and RSQLite has no such function. Coverage 45.7% -> 98.4% on this file.
Seven inline comments explained current behaviour by narrating the broken state before it. Past tense about code that no longer exists dates badly and makes a reader reconstruct history to find the rule. Rewritten in union_polygons, drainage_area_populated, .gpkg_drop_layer, gpkg_update_geom, hf_upstream_index, get_streamorder and .hf_ignore_lyrs. The add_measures comment restated the @details block rather than pointing at it. The long comments in geom.R, clean_geom.R, auth.R and the lake check stay: each documents external behaviour that cannot be read off the code.
No CODECOV_TOKEN secret is set, so codecov-action@v4 falls back to tokenless upload, which Codecov rate-limits. The upload step has never actually run: the coverage step failed ahead of it on every previous run, so this would have been its first execution.
Local lintr 3.2.0 reported clean; CI runs 3.3.0.1, which adds the two linters below. CONTRIBUTING now says to update lintr before trusting a local run. geom.R kept one magrittr pipe so the next call could use the `.` pronoun, which the native pipe has no equivalent for. Assign the intermediate instead and drop the now-unused %>% import. Swapping the pipe without restructuring would have reintroduced the "object '.' not found" failure recorded in 0.3.4. get_pfafstetter accumulated results with <<- from inside its recursion. Use an explicit environment, which is what the linter asks for and makes the mutable state visible. The 746-reach oracle test confirms the codes are unchanged.
0.4.0 and 0.4.1 were development versions on an unpushed branch, so nobody could install either. Three headings would have split the notes for the single v0.4.2 tag across versions that never existed. All 16 bullets are carried over unchanged; the lead says what the collapse covers.
Pushing during a long R-devel build left the old run competing with the new one for a runner; it happened three times on this branch, and the stale results do not count toward the PR head anyway. R-CMD-check, lint and test-coverage now cancel in progress. pkgdown keeps its job-level guard, which already exempts pull requests.
The file held four section comments and no code, naming the four files the work actually went into (OGRSQLDriver.R, OGRSQLConnection.R, OGRSQLResult.R, sf_ogr.R). It was never non-empty: a5c4d9f created it that way and nothing filled it in. Being in Collate meant it shipped and was sourced on every build, and a file called OGRSQL.R that does not define OGRSQL() sends anyone navigating by filename to the wrong place; the function is in sf_ogr.R. Nothing @includes it, and roxygen regenerates Collate from the @include directives in sf_ogr.R, so the load order that the S4 classes depend on is unchanged.
man/figures/README-unnamed-chunk-6-1.png came from a5c4d9f, the same commit that added the empty OGRSQL.R scaffold, and is referenced nowhere. Every chunk in the current README.Rmd is eval = FALSE apart from the library load, so no README figure is generated at all. It was still shipping 56K in the tarball, since man/figures is installed. The knitr fig.path is left pointing at man/figures/README-, so a future evaluated chunk still lands in the right place.
added 3 commits
August 8, 2026 18:58
read_qml(), create_style_row() and append_style() come across from the
hydrofabric package, together with QML for divides, flowlines,
flowpaths, hydrolocations, lakes and nexus. Styling is stamped into a
GeoPackage's layer_styles table at write time, so it belongs beside
write_hydrofabric() rather than in the build package, which does not
even import hfutils.
append_style() matched QML to layers by grepping the directory listing
and then pairing it against the caller's layer order positionally.
Those two orders are unrelated, so append_style(gpkg, layer_names =
c("nexus", "divides")) gave nexus the divides symbology and divides the
nexus one, silently. Match on exact basename and index by layer name
throughout. A requested layer with no shipped QML is skipped, and so is
an attribute table with no geometry column: create_style_row() indexed
row 1 of an empty gpkg_geometry_columns query and errored on network.
flowpaths line width was bound to `order` as a raw field, so stream
order was read straight as millimeters and an order-7 mainstem drew as
a 7 mm line. Drive it from tot_drainage_areasqkm instead, log-scaled
because drainage area spans six orders of magnitude, capped at 5 mm,
and render ascending so mainstems sit on top at confluences. Add
flowlines.qml for the reference linework: same attribute, thin and
light blue over a narrower range.
Not wired into write_hydrofabric(); calling it stays explicit.
Waterbodies had no symbology. WB is a distinct layer from lakes, sharing only id and geometry: lakes carries the NWM reservoir routing parameters (weir and orifice coefficients, dam length) and is styled as points, while WB is waterbody polygons. WB.qml is a semi-transparent pale blue fill so flowpaths stay legible beneath it. Every geometry-bearing table in the data model now ships a style: divides and WB as polygon, flowpaths and flowlines as line, hydrolocations, lakes and nexus as point. network and flowpath_attributes are attribute-only and take none. Tests assert both that coverage and that each file declares the geometry type its layer actually has, since a QML with the wrong layerGeometryType is silently ignored by QGIS.
Adds `styles`, defaulting to FALSE. layer_styles is a QGIS extension rather than part of the GeoPackage specification, so writing it unconditionally would add a table that every reader sees, not just as_ogr(): sf::st_layers(), ogrinfo and fiona all list it. It also costs a fixed ~70 KB, most of it divides.qml, which is a large fraction of a small subset. The failure modes are not symmetric. Forgetting to style a deliverable shows up the moment the file is opened and is fixable afterwards with append_style(). Styling a pipeline intermediate is silent and propagates into everything built from it. Off by default, on for deliverables. Applied after the atomic swap, on the finished file, so a styling failure warns and leaves a correctly written fabric instead of aborting one. Layers with no shipped QML and attribute tables with no geometry are skipped by append_style() already, so passing the whole layer set is safe.
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
1 participant
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.
Takes
mainfrom 0.3.4 to 0.4.2. 25 commits, clean fast-forward (no commits onmainare absent here).Correctness fixes
None of these were ever released, so no published version is affected.
hf_upstream_index()returned a degenerate index under its defaultnexus = NULL. Downstreams resolved only through the nexus map, so an empty map made every flowpath a terminal: each node isolated,num_upstreams0, no warning.write_hydrofabric()passesnex = NULLwhenever the written list has no nexus layer, so it stamped those values into the GeoPackage.as_ogr()'s layer filter was unanchored, silently dropping any real layer whose name merely containedgpkg_,rtree_orsqlite_(for exampleflowpaths_gpkg_v2). Now anchored, andlayer_styles/qgis_projectsare excluded so a QGIS-styled fabric does not read as ambiguous.gpkg_update_geom()leaked fourrtree_*tables per call into the user's GeoPackage, plus triggers and agpkg_extensionsrow. Ten updates left forty orphan tables. Cleanup is now complete and inside the same transaction.Coverage
First tests for
gpkg_update_col(),gpkg_update_geom()andgpkg_exec()— three exported functions that write destructively in place and had none.R/gpkg_metadata.R45.7% to 98.4%; package 76.2% to 81.6%. Suite is 326 tests.Delivery
pkgdown::check_pkgdown()was failing on three exports missing from the reference index, which fails the site build outright. The last green Website run predates those functions, so the break was queued for the next push tomain.add_measures()join key.LINTR_ERROR_ON_LINT=true; spelling now fails the suite on a new word rather than scrolling past.Local verification
R CMD check --as-cranclean apart from an "unable to verify current time" NOTE, which is a sandbox artifact. Verified on macOS with R 4.4.2 only — this PR exists so the five-config matrix runs before anything reachesmain.Note that
Websitebuilds but does not deploy on a pull request; that is expected.