Investigate: Claude claims soil-id-algorithm may drop depth interval data

[knipec]

Claude asserts that data gets dropped by the soil-id-algorithm unless intervals are contiguous and start from 0.

I had it write tests displaying the behavior: test_depth_boundary.py

Note: Even after fixing this, we expect data would get dropped at depths greater than 200cm. This cap is only is relevant once we increase the max depth, and we may want to consider communicating it to the user?

More Claude analysis here

[johannesparty]

Corrected analysis — confirmed, with two important refinements

I verified the claim against the actual source (not just the mirror) and by running the attached test_depth_boundary.py. The bug is real, but two things in the framing need correcting.

The bug is real, and the mechanism is a list-position vs depth-index misalignment

In rank_soils_global (global_soil.py#L637-L643) the user's horizon values are packed into a contiguous list with no depth alignment:

p_sandpct_intpl = [spt[i] for i in range(len(soilHorizon)) for _ in range(top[i], bottom[i])]

Then (global_soil.py#L687) the list is filtered by depth index:

p_hz_data = p_hz_data[p_hz_data.index.isin(pedon_slice_index)]

pedon_slice_index holds true depths (e.g. [0..24, 100..149]), but the second interval's values sit at list positions 25..74, not at index 100..149. The filter therefore reads NaN where the deeper data should be, and that data is silently dropped. Running the attached test confirms it exactly:

shallow_then_gap_then_deep: intervals [(0,25,A),(100,150,B),(200,210,C)]
  A found at depths 0..24      ✓ (only because it starts at 0)
  B (entered for 100-149)      ✗ → NaN, value lost
  → 25 rows kept a value, 50 rows NaN

3 failed / 6 passed; the 6 that pass are all contiguous-from-0 cases. So: a gap — or a first interval that doesn't start at 0 — silently drops/misaligns everything below it. A non-zero start behaves like a leading gap.

Refinement 1: the US path (rank_soils) is already fixed

The test header says us_soil.rank_soils has a "structurally identical block." It doesn't anymore — it was rewritten to be depth-indexed (us_soil.py#L1655-L1669):

sand_array = [np.nan] * max_user_depth
for d in range(t, b):
    sand_array[d] = spt[i]      # value placed at actual depth d

This keeps list position == depth, so the US path is not affected. Only the global path (rank_soils_global) still has the bug. We should confirm which users actually route through the global path.

Refinement 2: this is NOT the same bug as #184

They're independent bugs in different functions:

• bug: algorithm crash KeyError: $SOIL_NAME #184 is a crash (KeyError) in soil_sim — the surface aggregate bucket is dropped by .dropna() (soil_sim.py#L144).
• This is a silent wrong answer in rank_soils_global — misaligned user data produces a degraded/incorrect match with no crash.

They share only the theme "depth handling is fragile." Fixing one does nothing for the other.

Sketch of the fix (mirror what rank_soils already does)

Replace the contiguous packing + adjust_depth_interval block in rank_soils_global with a depth-indexed build:

spt = [getSand(sh) for sh in soilHorizon]
cpt = [getClay(sh) for sh in soilHorizon]
p_cfg = [getCF_fromClass(rf) for rf in rfvDepth]

# Place each horizon's value at its TRUE depth index (0..199) so list
# position == depth and the later index.isin(pedon_slice_index) filter aligns.
sand_array = [np.nan] * 200
clay_array = [np.nan] * 200
cfg_array  = [np.nan] * 200
for i in range(len(soilHorizon)):
    t, b = top[i], min(bottom[i], 200)
    for d in range(t, b):
        sand_array[d] = spt[i]
        clay_array[d] = cpt[i]
        cfg_array[d]  = p_cfg[i]

p_sandpct_intpl = pd.DataFrame(sand_array)
p_claypct_intpl = pd.DataFrame(clay_array)
p_cfg_intpl     = pd.DataFrame(cfg_array)
# drop the three adjust_depth_interval() calls — arrays are already length 200

One related cleanup to fold in: pedon_slice_index is built from soil_df_slice while the packed lists are built from soilHorizon/top/bottom derived from soil_df — two frames that can differ in row count after the dropna(subset=...). Building depth-indexed arrays makes the result robust to that, but the value source and the index source should be made consistent as part of the fix.

After the fix, gaps and non-zero starts are handled truthfully: gap depths become NaN (= "no data here," excluded from the similarity calculation), which is the correct representation. The separate 200 cm truncation noted in the issue is a real, distinct limitation (see pedon_slice_index < 200 cutoff) and is by-design until max depth is raised.

On warning users about gaps

I'd recommend fixing the alignment rather than warning users to avoid gaps:

• Once fixed, gaps are correct (the gap depths simply don't contribute), so no warning is needed for correctness.
• Telling users to avoid gaps pushes them to fabricate contiguous horizons, which degrades data quality — worse than the gap.
• Gaps and non-zero starts are legitimate field reality (you sample what you can reach).

A soft, non-blocking note ("we'll treat un-sampled depths as no data") could be reasonable UX, and a temporary warning is defensible as a stopgap until the fix ships — but a warning should not be the fix.