Inconsistent and trap-laden conversion paths from `hy_node` to `hy_topo` for non-dendritic input
- Dominant language
- R
- Stars
- 35
- Forks
- 5
- Avg merge
- 21m
- Merged PRs (30d)
- 2
Description
## Problem
When a user starts from a raw `fromnode`/`tonode` edge list with no divergence
information — common with international hydrography datasets (AGF, EU-Hydro,
NHN, GRIT) where there is no primary-vs-secondary flag — the path to a
`hy_topo` object is inconsistent across functions and has no non-deprecated
route.
Concrete failure: `get_bridge_flowlines()` on raw `fromnode`/`tonode` data
produces
```
Error: get_bridge_flowlines() requires hy_topo.
Current input is: hy_node.
Use add_toids() to convert fromnode/tonode to edge list.
```
Following the guidance leads to a circular dead-end:
- `add_toids(x)` (default `return_dendritic = TRUE`) → errors with *"To remove
non dendritic paths, a divergence attribute is required."*
- `add_toids(x, return_dendritic = FALSE)` → works, but emits a deprecation
warning telling the user to *"Use `to_flownetwork()` for non-dendritic edge
lists."*
- `to_flownetwork.hy_node()` → errors and tells the user to *"Use
`add_toids()` then `add_levelpaths()` to enrich the network"* — both of
which need `divergence`, which is exactly what we don't have.
- `add_divergence()` requires outlet ids, which the user may not have for
arbitrary international datasets.
## Inconsistency between functions
For raw `hy_node` input, sibling functions behave differently:
| Function | hy_node behavior |
| ------------------------- | ------------------------------------------------------- |
| `sort_network()` | Auto-converts via `hy_node_to_topo()` → `add_toids()` (defaults to dendritic, requires divergence) |
| `add_streamorder()` | Auto-converts via `hy_node_to_topo()` (same trap) |
| `add_pathlength()` | Errors, instructs `add_toids()` |
| `get_bridge_flowlines()` | Errors, instructs `add_toids()` |
Beyond the dispatch inconsistency, the underlying `add_toids()` default is
wrong for the operations that actually want the non-dendritic graph.
**Bridge detection by definition requires the full undirected edge set** —
`get_bridge_flowlines.hy_topo()` even calls `make_nondendritic_topology()`
internally to reconstruct it. So `add_toids(return_dendritic = TRUE)` is the
wrong conversion for bridges even when divergence info is available.
## Suggested resolution
A safe set of assumptions and rules:
1. **Define a canonical "no-decision-needed" conversion.** A `hy_node` →
`hy_topo` path that does not require `divergence` and does not lose edges.
Options:
- Un-deprecate `add_toids(return_dendritic = FALSE)`, or
- Introduce `add_toids(return_dendritic = "auto")` that defaults to
non-dendritic when no `divergence` column exists, dendritic when one is
present.
2. **Make `to_flownetwork.hy_node()` work without `levelpath`** when the
caller only needs `id`/`toid` (i.e., dropping the `upmain`/`downmain`
fields). The current "needs divergence + levelpath" requirement is
incompatible with the deprecation message that points users at it from
`add_toids()`.
3. **Make conversion consistent across functions.** For functions that
operate on the *non-dendritic* edge set (`get_bridge_flowlines`, anything
that calls `make_nondendritic_topology` internally), the `hy_node` method
should auto-convert via the non-dendritic path defined in (1), not error.
For functions that genuinely require the dendritic edge list (e.g.,
levelpaths), keep the explicit error and document the `divergence`
requirement at the top of the man page.
4. **Improve the error guidance.** When a function errors because
`divergence` is missing, the message should explicitly say "your data has
no divergence column; either supply one or call
`add_divergence(coastal_outlet_ids = ..., inland_outlet_ids = ...)` first"
rather than pointing at a function that has the same precondition.
5. **Document the decision tree in `vignettes/non-dendritic.Rmd`** — a small
table mapping (`raw fromnode/tonode`, `+ divergence`, `+ levelpath`) →
(`which functions you can call`).
## Reproducer
```r
edges <- data.frame(
id = 1:5,
fromnode = c("a", "b", "c", "b", "d"),
tonode = c("b", "c", "e", "d", "c")
)
hydroloom::get_bridge_flowlines(edges)
#> Error: get_bridge_flowlines() requires hy_topo. ... Use add_toids() ...
hydroloom::add_toids(edges)
#> Error: To remove non dendritic paths, a divergence attribute is required.
hydroloom::to_flownetwork(edges)
#> Error: to_flownetwork() requires hy_leveled. ... Use add_toids() then add_levelpaths() ...
# Only working path — emits deprecation warning:
suppressWarnings(hydroloom::add_toids(edges, return_dendritic = FALSE)) |>
hydroloom::get_bridge_flowlines()
```
## Context
Encountered while porting a divergence-density analysis for AGF, EU-Hydro, NHN, GRIT from `igraph` to
hydroloom. The four datasets all provide `id` + `fromnode` + `tonode` with
no divergence flag; the analysis needs bridges and post-bridge connected
components, both of which require `hy_topo`.
*Drafted with assistance from Claude Opus 4.7*
Contributor guide
Assessment
This issue has not been assessed yet.