Clarify indexing by SouthEndMusic · Pull Request #886 · Deltares/Wflow.jl

SouthEndMusic · 2026-04-23T14:45:27Z

Issue addressed

Fixes #863

Explanation

Explain how you addressed the bug/feature request, what choices you made and why.

Checklist

Updated tests or added new tests
Branch is up to date with master
Tests & pre-commit hooks pass
Updated documentation if needed
Updated changelog.qmd if needed

SouthEndMusic · 2026-04-23T14:46:28Z

@copilot can you review this PR?

Copilot

Pull request overview

This PR addresses #863 by clarifying ambiguous index/count variable names across the codebase (e.g., n, i, order, indices) and aligning related struct fields and call sites to the more descriptive naming.

Changes:

Renamed many generic loop indices and size/count fields to domain-specific names (e.g., n_land_cells, n_river_cells, cell_order, land_indices_2d).
Updated network/domain field names and propagated those changes through routing, hydrology, sediment, and IO code.
Updated unit/integration tests to use the new names and fields.

Reviewed changes

Copilot reviewed 54 out of 54 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
Wflow/test/water_demand.jl	Renames `n`/indices in water demand tests to `n_land_cells` and updates network field names.
Wflow/test/testing_utils.jl	Renames loop indices and updates soil init helper arg name to `n_land_cells`.
Wflow/test/subdomains.jl	Updates test references from `order` to `cell_order`.
Wflow/test/soil.jl	Renames `n` to `n_land_cells` throughout soil unit tests and related model constructors.
Wflow/test/sediment.jl	Renames river cell counts/indices and updates river network `order` → `cell_order`.
Wflow/test/run_sediment.jl	Updates indexing into river network from `order` to `cell_order`.
Wflow/test/run_sbm_piave.jl	Renames river→land index mapping variable to more descriptive name.
Wflow/test/run_sbm_gwf.jl	Renames river→land index mapping variable and updates `order` → `cell_order` access.
Wflow/test/run_sbm.jl	Updates network order field name and clarifies land cell index variable names.
Wflow/test/routing_process.jl	Clarifies river cell/edge index variables in routing tests.
Wflow/test/reservoir.jl	Renames reservoir count `n` → `n_reservoirs` in tests.
Wflow/test/io.jl	Renames land index mapping variable `indices` → `land_indices_2d` in IO tests.
Wflow/test/groundwater.jl	Renames land/river cell counts and updates constructors to `n_land_cells` / `n_river_cells`.
Wflow/src/vegetation/parameters.jl	Renames `indices` → `land_indices_2d` and `n` → `n_land_cells` for vegetation parameter loading.
Wflow/src/vegetation/canopy.jl	Renames interception variable dimension field to `n_land_cells` and updates loop index naming.
Wflow/src/surfacewater/runoff.jl	Renames land/river index variables and improves clarity in river-depth mapping loop.
Wflow/src/subdomains.jl	Renames loop indices and counters for clearer subbasin/subdomain logic.
Wflow/src/soil/soil_process.jl	Renames soil layer loop index for clarity.
Wflow/src/snow/snow.jl	Renames dimension fields and index variables (`n_land_cells`) across snow model structs and updates.
Wflow/src/sediment_model.jl	Updates land index mapping field name usage for sediment model initialization.
Wflow/src/sediment_flux.jl	Updates index field names (`land_indices_2d`, `river_indices_2d`) and clarifies type parameter formatting.
Wflow/src/sediment/sediment_transport/overland_flow_transport.jl	Renames `n` → `n_land_cells` for overland sediment transport model structs and constructors.
Wflow/src/sediment/sediment_transport/land_to_river.jl	Renames river dimension to `n_river_cells` and updates update loops accordingly.
Wflow/src/sediment/erosion/soil_erosion.jl	Renames `n` → `n_land_cells` and updates threaded loops to use descriptive indices.
Wflow/src/sediment/erosion/river_erosion.jl	Renames `n` → `n_river_cells` and updates threaded loops accordingly.
Wflow/src/sediment/erosion/rainfall_erosion.jl	Renames `n` → `n_land_cells`, updates parameter reads to `land_indices_2d`, and clarifies loop indices.
Wflow/src/sediment/erosion/overland_flow_erosion.jl	Renames `n` → `n_land_cells`, updates reads to `land_indices_2d`, and clarifies loop indices.
Wflow/src/sediment/erosion/erosion_process.jl	Renames function parameter `v` → `river_cell_idx` for clarity.
Wflow/src/sbm_model.jl	Clarifies `n_river_cells` and land-cell loop indices during state setting and storage init.
Wflow/src/sbm.jl	Renames indices to `land_indices_2d` and `n` → `n_land_cells` in model initialization paths.
Wflow/src/routing/utils.jl	Renames `order` → `cell_order` and clarifies downstream count naming in accumulation helpers.
Wflow/src/routing/timestepping.jl	Renames timestep array sizing input to `n_land_cells`.
Wflow/src/routing/surface/surface_process.jl	Renames loop index in kinematic wave surface routine to `land_cell_idx`.
Wflow/src/routing/surface/surface_kinwave.jl	Renames dimension fields (`n_cells`, `n_land_cells`, `n_river_cells`) and updates constructors/call sites.
Wflow/src/routing/surface/reservoir.jl	Renames outlet indices field and clarifies reservoir indices in loops and helper functions.
Wflow/src/routing/subsurface/lateral_subsurface_flow.jl	Renames `n` → `n_land_cells` and updates model initialization and loop variables for clarity.
Wflow/src/routing/subsurface/groundwater.jl	Renames index variables, updates network fields, and clarifies conductance/flux loops.
Wflow/src/routing/subsurface/connectivity.jl	Renames sparse-matrix loop variables (`nnz_idx`, `col_idx`) for clarity.
Wflow/src/routing/subsurface/boundary_conditions.jl	Renames index variables and updates boundary-condition model dimension fields.
Wflow/src/network.jl	Renames key network fields (`indices`→`land_indices_2d`, `order`→`cell_order`, etc.) and related mapping vectors.
Wflow/src/mass_balance.jl	Renames index variables in mass balance computations for land/river/reservoir and routing balances.
Wflow/src/io.jl	Renames loop indices in forcing and output handling for clarity.
Wflow/src/glacier/glacier.jl	Renames glacier dimension field to `n_land_cells` and updates update loops accordingly.
Wflow/src/forcing.jl	Renames `AtmosphericForcing.n` → `n_land_cells` and `HydrologicalForcing.n` → `n_cells`.
Wflow/src/erosion.jl	Updates erosion model initialization to use `land_indices_2d` and `n_land_cells`.
Wflow/src/domain.jl	Updates domain/network field names and associated indexing into land/river/reservoir masks.
Wflow/src/bmi.jl	Renames loop indices and clarifies variable naming in BMI grid/value helpers.
Wflow/src/Wflow.jl	Renames timestep loop/index variables and CLI arg count variable for clarity.

Comments suppressed due to low confidence (2)

Wflow/src/routing/subsurface/lateral_subsurface_flow.jl:143

NetworkDrain was updated to expose drain_indices_2d (not indices). This call still uses drain.network.indices, which will fail at runtime with an unknown field error. Use the updated field name (and ensure DrainageModel expects the 2D drainage indices).

    end
    if config.model.drain__flag
        drain = DrainageModel(dataset, config, drain.network.indices)
    else
        drain = nothing

Wflow/src/sbm.jl:33

n is no longer defined in this function (it was renamed to n_land_cells), so RutterInterceptionModel(vegetation_parameters, n) will raise an UndefVarError. Pass n_land_cells instead to match the updated parameter naming/signature.

        @info "Using the Gash interception model since dt >= 23 hours."
    else
        interception = RutterInterceptionModel(vegetation_parameters, n)
        @info "Using the modified Rutter interception model since dt < 23 hours."
    end

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-28T06:39:57Z

-                            land_v.h[i] * x_length[i] * y_length[i] + bankfull_storage[j]
+            for land_cell_idx in eachindex(land_v.storage)
+                if river_location[land_cell_idx]
+                    river_cell_idx = domain.land.network.river_indices[land_cell_idx]


NetworkLand renamed the land→river mapping vector from river_indices to land_cell_river_indices. This line still references domain.land.network.river_indices[...], which will cause a field error. Update it to the new field name so local-inertial storage initialization works.

Suggested change

river_cell_idx = domain.land.network.river_indices[land_cell_idx]

river_cell_idx =

domain.land.network.land_cell_river_indices[land_cell_idx]

Copilot · 2026-04-28T06:39:57Z

+    graph = DiGraph(n_river_cells)
+    for river_cell_idx in 1:(n_river_cells)
+        add_edge!(graph, river_cell_idx, river_cell_idx + 1)
    end


The graph construction adds an edge from the last river cell to n_river_cells + 1, but the graph only has n_river_cells vertices. This will throw an error (invalid vertex) and breaks the test. The edge loop should stop at n_river_cells - 1 (or construct the graph with n_river_cells + 1 if that was intended).

Copilot · 2026-04-28T06:39:58Z

-    land_indices::Vector{Int} = 1:length(indices)
+    land_indices_2d::Vector{CartesianIndex{2}} = CartesianIndex{2}[]
+    # land cell indices
+    land_cell_indices::Vector{Int} = eachindex(land_indices_2d)


land_cell_indices is declared as Vector{Int} but its default value is eachindex(land_indices_2d), which returns a Base.OneTo range, not a Vector. This will cause a type error when constructing NetworkLand via the keyword constructor (including in NetworkLand(dataset, config) where this field isn't passed). Consider changing the field type to AbstractVector{Int}/UnitRange{Int} or using collect(eachindex(land_indices_2d)) for the default.

Suggested change

land_cell_indices::Vector{Int} = eachindex(land_indices_2d)

land_cell_indices::Vector{Int} = collect(eachindex(land_indices_2d))

vers-w · 2026-05-13T07:38:37Z

General comment: I think it is a good idea to clarify indexing, however to my taste it is now a bit too explicit resulting in a less generic approach. I don't think all the river_ and land_ prefixes have much added value. Also when we decide for example to define a glacier model only for glacier cells (similar to river or reservoir domain) are we then changing the prefix to glacier_? Furthermore, I would say it follows implicitly from the object to what the index refers to, and if that's not the case then it would be good to use an explicit name. Two examples to illustrate:

model.domain.land.network.land_cell_indices
model.domain.land.network.land_cell_river_indices

# replace by:

model.domain.land.network.cell_indices # cell indices refer to `land.network`
model.domain.land.network.river_cell_indices # prefix `river_` required as it refers to the river domain

"Struct for storing glacier model variables"
@with_kw struct GlacierVariables
    n_land_cells::Int

# replace by:

"Struct for storing glacier model variables"
@with_kw struct GlacierVariables
    number_of_cells::Int

Also tagging @JoostBuitink for his input on this.

JoostBuitink · 2026-05-13T09:30:52Z

Also tagging @JoostBuitink for his input on this.

I agree with your suggestion, as that makes the code a bit cleaner but still much more readable than the old i, j, k variables for indices!

SouthEndMusic · 2026-05-13T12:24:09Z

@vers-w I agree with your first suggestion, as the river cells are a subset of what I now call land cells, which I only realized later.

Also when we decide for example to define a glacier model only for glacier cells (similar to river or reservoir domain) are we then changing the prefix to glacier_?

That's absolutely the idea; each enumeration of cells (or other objects for that matter) should have its own unique and globally used name.

Furthermore, I would say it follows implicitly from the object to what the index refers to

I strongly oppose this approach. it's a relatively low effort for longtime developers to make the code accessible to new developers. Since we've had similar disagreements elsewhere (e.g. #735 (comment)), I think we should have a broader discussion about this.

SouthEndMusic · 2026-05-13T12:38:07Z

Another approach would be to do something similar to Ribasim, creating a CellMetadata object per cell which holds all the different indices the cell has in different modules

vers-w · 2026-05-13T12:56:11Z

Another approach would be to do something similar to Ribasim, creating a CellMetadata object per cell which holds all the different indices the cell has in different modules

Hm, I would say that info is stored in the domain/network part of Wflow. That's why I think it is perfectly fine to not have these prefixes for indexing as part of objects that refer to a river, reservoir, drain etc, and I think also @JoostBuitink agrees on this based on his comment.

Regarding #735 (comment), both @JoostBuitink and I think all these unit comments do not have real added value (I think these are even distracting, it is mostly about knowing what are flux and store parameters/variables and if it is unclear from the naming checking struct/function annotations or the docs is perfectly fine). We can of course discuss this further. In the end I think it is always useful that a new contributor besides checking computations also checks struct and function annotations and the documentation, this is in my view the best way to learn about a new code base.

vers-w · 2026-05-18T13:32:21Z

-            g_ttm[i],
-            g_cfmax[i],
-            g_sifrac[i],
+    threaded_foreach(1:n_cells; basesize = 1000) do land_cell_idx


As mentioned previously, I don't think these land_ prefixes (and river_ at other places in the code) add much value. I would strongly recommend to just use idx or i here and at other places in the code where applicable. There is a loop for n_cells which is defined as part of glacier, how idx is linked to the catchment/land domain is store as part of the network data, we do not have to specify that here. It also keeps it more generic in case we want to define a glacier only for actual glacier cells.

I agree with land_cell_idx => cell_idx. I do think leaving the river_ prefix is useful, especially in places where domains are mixed, like this:

Wflow.jl/Wflow/src/sbm.jl

Lines 158 to 167 in de24b20

for (river_cell_idx, cell_idx) in

enumerate(domain.river.network.cell_indices_containing_river)

total_storage[cell_idx] = (

(

river_flow.variables.h[river_cell_idx] *

flow_width[river_cell_idx] *

flow_length[river_cell_idx]

) / (area[cell_idx]) * 1000 # Convert to mm

)

end

Yes, I fully agree that in places where domains mix this is very useful! Other than that, I would remove it, including for example n_river_cells, n_river_edges as part of structs or for example here:

Wflow.jl/Wflow/src/routing/surface/surface_local_inertial.jl

Lines 271 to 274 in de24b20

river_v.zs_src[river_edge_idx] =

river_p.zb[river_cell_idx_src] + river_v.h[river_cell_idx_src]

river_v.zs_dst[river_edge_idx] =

river_p.zb[river_cell_idx_dst] + river_v.h[river_cell_idx_dst]

OK, I did the following renames where possible:

*_cell_idx => cell_idx

n_*_cells => n_cells

n_*_edges => n_edges

OK, I did the following renames where possible:

* `*_cell_idx => cell_idx` * `n_*_cells => n_cells` * `n_*_edges => n_edges`

Thanks for these changes! I am not completely convinced if using cell_idx is very useful (also discussed briefly with @JoostBuitink ), it does not add much clarification and is not very generic. I would suggest to just use idx unless there is confusing in a loop about for example the domain (river vs. land).

I would say that cell_idx is useful, it differentiates from e.g. edge_idx, and it will always be the index of a cell right?

…aster)

Rename generic loop variables to descriptive names: - i -> cell_idx, river_cell_idx, reservoir_idx, etc. - n -> n_cells, n_river_cells, n_reservoirs, etc. - m/k -> soil_layer_idx - i_src/i_dst -> river_cell_idx_src/river_cell_idx_dst - n -> n_active_flood_edges (floodplain context) - i -> river_edge_idx, flood_edge_idx (edge iteration) - i0 -> depth_count

Rename n::Int to n_cells::Int in land-domain structs and n_river_cells::Int in river-domain structs across all model files. Also n_reservoirs::Int for reservoir structs. Affected files: water_demand.jl, glacier.jl, snow.jl, runoff.jl, canopy.jl, lateral_subsurface_flow.jl, surface_kinwave.jl, surface_local_inertial.jl, reservoir.jl, groundwater.jl, boundary_conditions.jl, and all sediment files. Test files updated to pass renamed keyword arguments.

vers-w

I did notice that the index i has been replaced by idx. If that is the preferred term it is fine with me, although for me i (it is a bit shorter) is also fine.

Generally, the comments are about explicit naming of indices that are in my opinion not always required/ a bit too explicit. For example, I do not see the added value of using river_idx (or river_edge_index) for river objects, unless an additional object (e.g. reservoir or land) is handled in the same function.

vers-w · 2026-06-22T11:54:57Z

-    actual_surfacewater_abstraction_volume::Vector{Float64} = zeros(n)   # actual surface water abstraction [m³ s⁻¹]
-    available_surfacewater::Vector{Float64} = zeros(n)      # available surface water [m³]
-    non_irrigation_returnflow::Vector{Float64} = zeros(n)          # return flow from non irrigation [m s⁻¹]
+    n_river::Int


Suggest to keep n here (and where this is applicable). This is in line with land objects, and from the struct name (and doc string) it is already clear this is about a river object.

Suggested change

n_river::Int

n::Int

vers-w · 2026-06-22T12:13:05Z

+    nnz_idx = 1  # column index of sparse matrix
+    col_idx = 1  # row index of sparse matrix


Are these logical names for the index (based on the comments)?

vers-w · 2026-06-22T12:23:50Z

    (; river) = subsurface_flow_model.boundary_conditions

    ns = length(order_of_subdomains)
    for hydraulic_conductivity in 1:ns


How is hydraulic_conductivity related to the order_of_subdomains?

vers-w · 2026-06-22T12:39:43Z

-        if linked_reslocs[i] > 0
-            parameters.lower_reservoir_ind[i] =
-                only(findall(x -> x == linked_reslocs[i], reslocs))
+    for reservoir_idx in 1:n_reservoirs


I think it would be fine to use here only idx (or i), as this is already clear from n_reservoirs.

vers-w · 2026-06-22T12:40:21Z

 "Struct for storing reservoir model boundary conditions"
 @with_kw struct ReservoirBC
-    n::Int
+    n_reservoirs::Int


Also suggested for river objects:

Suggested change

n_reservoirs::Int

n::Int

vers-w · 2026-06-22T13:30:03Z

-        for j in nbs
-            if ldd[j] != ldd[i]
-                fraction[j] = slope[j] / (slope[i] + slope[j])
+    for river_idx in inds_river


Suggested change

for river_idx in inds_river

for idx in inds_river

vers-w · 2026-06-22T13:31:10Z

-    for i in 1:nv(graph)
-        push!(src_edge, findall(isequal(nodes[i]), nodes_at_edge.dst))
-        push!(dst_edge, findall(isequal(nodes[i]), nodes_at_edge.src))
+    for node_idx in 1:nv(graph)


Suggested change

for node_idx in 1:nv(graph)

for idx in 1:nv(graph)

vers-w · 2026-06-22T13:33:29Z

-    n = nv(graph)
-    for (i, v) in enumerate(pits)
+    n_vertices = nv(graph)
+    for (pit_idx, pit_node) in enumerate(pits)


Suggested change

for (pit_idx, pit_node) in enumerate(pits)

for (idx, node_idx) in enumerate(pits)

vers-w · 2026-06-22T13:33:54Z

    n = length(we_x)
-    for v in toposort
-        dst = outneighbors(graph, v)
+    for node_idx in toposort


Suggested change

for node_idx in toposort

for idx in toposort

vers-w · 2026-06-22T13:35:01Z

    runstart_time = now()
-    @progress for (i, time) in enumerate(times)
-        @debug "Starting timestep." time i now()
+    @progress for (timestep_idx, time) in enumerate(times)


Suggested change

@progress for (timestep_idx, time) in enumerate(times)

@progress for (idx, time) in enumerate(times)

Clarify indexing

0ba5014

Clarify more indexing

e7cea9b

SouthEndMusic requested a review from Copilot April 28, 2026 05:59

Copilot started reviewing on behalf of SouthEndMusic April 28, 2026 06:35 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

SouthEndMusic added 3 commits April 28, 2026 09:37

Comments addressed

8a9a719

Fix single thread test

8dafc32

Merge master

049fd80

This was referenced May 11, 2026

Support kinematic wave floodplain routing #870

Open

Use SI units for internal computations #735

Merged

SouthEndMusic added 2 commits May 18, 2026 09:58

Merge master

137c386

Comments addressed

06acf2c

SouthEndMusic requested a review from vers-w May 18, 2026 08:21

vers-w reviewed May 18, 2026

View reviewed changes

SouthEndMusic added 9 commits May 18, 2026 16:08

land_cell_idx => cell_idx

de24b20

Generalize names

ab33eaa

Fix

c4ff7a8

Merge branch 'master' into clarify_indexing

a4bd7b8

Merge branch 'master' into clarify_indexing (resolve conflicts with m…

a2f1767

…aster)

Fixes

a7a17c8

Fixes

30a4ac6

SouthEndMusic added 4 commits June 11, 2026 09:44

Merge master

593aaf3

Generalize some indexing

0c2927c

Generalize some more indexing

7f1676f

nit

1e3888c

SouthEndMusic requested a review from vers-w June 11, 2026 08:16

SouthEndMusic added 3 commits June 11, 2026 10:27

Merge branch 'master' into clarify_indexing

c78cabb

Test fix

79e62b0

Make prek happy

e8066f1

vers-w requested changes Jun 22, 2026

View reviewed changes

	river_cell_idx = domain.land.network.river_indices[land_cell_idx]
	river_cell_idx =
	domain.land.network.land_cell_river_indices[land_cell_idx]

	land_cell_indices::Vector{Int} = eachindex(land_indices_2d)
	land_cell_indices::Vector{Int} = collect(eachindex(land_indices_2d))

	for (river_cell_idx, cell_idx) in
	enumerate(domain.river.network.cell_indices_containing_river)
	total_storage[cell_idx] = (
	(
	river_flow.variables.h[river_cell_idx] *
	flow_width[river_cell_idx] *
	flow_length[river_cell_idx]
	) / (area[cell_idx]) * 1000 # Convert to mm
	)
	end

	river_v.zs_src[river_edge_idx] =
	river_p.zb[river_cell_idx_src] + river_v.h[river_cell_idx_src]
	river_v.zs_dst[river_edge_idx] =
	river_p.zb[river_cell_idx_dst] + river_v.h[river_cell_idx_dst]

		nnz_idx = 1 # column index of sparse matrix
		col_idx = 1 # row index of sparse matrix

	for (pit_idx, pit_node) in enumerate(pits)
	for (idx, node_idx) in enumerate(pits)

	@progress for (timestep_idx, time) in enumerate(times)
	@progress for (idx, time) in enumerate(times)

Uh oh!

Conversation

SouthEndMusic commented Apr 23, 2026

Issue addressed

Explanation

Checklist

Uh oh!

SouthEndMusic commented Apr 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

vers-w commented May 13, 2026

Uh oh!

JoostBuitink commented May 13, 2026

Uh oh!

SouthEndMusic commented May 13, 2026

Uh oh!

SouthEndMusic commented May 13, 2026

Uh oh!

vers-w commented May 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vers-w left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants