Skip to content

Fix _parse_rat to pad category_names to pixel indices for sparse RATs#3643

Merged
brendancol merged 3 commits into
xarray-contrib:mainfrom
Melissari1997:issue-3591
Jul 7, 2026
Merged

Fix _parse_rat to pad category_names to pixel indices for sparse RATs#3643
brendancol merged 3 commits into
xarray-contrib:mainfrom
Melissari1997:issue-3591

Conversation

@Melissari1997

Copy link
Copy Markdown
Collaborator

Closes #3591.

When a PAM sidecar's thematic RAT has sparse or non-zero-based values (e.g. [1, 2, 5] from QGIS/GDAL classified rasters), _parse_rat was sorting rows and returning a dense list, misaligning category names against their pixel values. The attrs contract promises index == pixel value.

  • Pad category_names with empty strings and category_colors with (0,0,0,0) for gaps between sparse RAT rows so list index == pixel value.
  • Fail closed on negative pixel values or a max value exceeding 1,000,000 to prevent OOM from pathological RATs.

Backend coverage: backend-agnostic — runs during open_geotiff metadata attach on all backends (numpy, dask, cupy, dask+cupy).

Test plan

  • Existing dense 0-based RAT tests pass unchanged.
  • Sparse 1-based RAT produces padded, index-aligned names.
  • Sparse RAT with RGBA columns pads both names and colors.
  • Negative values fail closed (return empty dict from read_pam_sidecar).
  • Values >= 1M fail closed.
  • No Value column uses Row index attributes and pads.
  • test_field_defn_without_usage_is_skipped still passes.

…array-contrib#3591)

When a sidecar's RAT has a sparse or non-zero-based Value column
(e.g. values [1, 2, 5] from QGIS/GDAL), _parse_rat now pads
category_names and category_colors with empty strings / (0,0,0,0)
so list index equals pixel value, matching the attrs contract.

Fails closed on negative values and values >= 1,000,000 to avoid
OOM from pathological RATs.

@Melissari1997 Melissari1997 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Fix _parse_rat to pad category_names to pixel indices for sparse RATs

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • _pam.py:241-242 -- max(r[0] for r in rows) / min(r[0] for r in rows) iterate rows twice. rows is typically small (a handful of RAT classes), so this is negligible in practice, but computing both in a single pass over rows (e.g. manual loop or tee) would be trivially cleaner. Not blocking.

Nits (optional improvements)

  • _pam.py:194 -- _MAX_CATEGORIES = 1_000_000 is module-level but _parse_rat is the only consumer. If you prefer keeping the guard close to the code it protects, it could be a local constant inside _parse_rat. Style preference, no functional difference.

What looks good

  • The fix correctly aligns the output with the documented category_names contract ("index == pixel value").
  • Fails closed on the two known OOM hazards (negative values, pathological max) rather than silently allocating.
  • The _MAX_CATEGORIES guard (1M) gives ample headroom for realistic classified rasters while preventing genuine OOM.
  • Six well-structured tests cover: the exact repro from #3591, color padding, negative guard, max guard, no-Value-column path with sparse Row indices, and regression on dense 0-based RATs.
  • All 10 tests in the file pass, including the 4 pre-existing reader-edge tests.
  • The change is surgical -- only _parse_rat is modified; build_pam_xml, read_pam_sidecar, and callers are untouched.
  • Backend coverage is correctly described as backend-agnostic (runs during open_geotiff metadata attach on all backends).

Checklist

  • Algorithm matches reference/paper -- N/A (metadata parser fix, not a numerical algorithm)
  • All implemented backends produce consistent results -- N/A (backend-agnostic metadata reader)
  • NaN handling is correct -- N/A (no numerical computation)
  • Edge cases are covered by tests -- negative values, huge values, sparse values, no Value column, dense 0-based regression all tested
  • Dask chunk boundaries handled correctly -- N/A
  • No premature materialization or unnecessary copies -- no .values/.compute() calls
  • Benchmark exists or is not needed -- not needed for a metadata parser
  • README feature matrix updated -- N/A (no new public API)
  • Docstrings present and accurate -- _parse_rat docstring updated to reflect new padding behavior

@Melissari1997 Melissari1997 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up review: disposition of findings

Suggestion: single-pass min/max (_pam.py:241-242)

  • Fixed in 53d4a2f. Replaced max()/min() with a single for r in rows: loop computing both in one pass.

Nit: _MAX_CATEGORIES module-level vs function-local (_pam.py:194)

  • Dismissed. Module-level constants are the established convention in this file: _USAGE_MINMAX, _USAGE_NAME, _USAGE_RED, _USAGE_GREEN, _USAGE_BLUE, _USAGE_ALPHA, _TYPE_INT, _TYPE_STRING are all at module level (lines 24-32). Keeping _MAX_CATEGORIES alongside them is consistent with existing style.

All 10 tests continue to pass.

@brendancol brendancol self-requested a review July 7, 2026 18:51
@brendancol

Copy link
Copy Markdown
Contributor

@Melissari1997 wooohooo

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brendancol brendancol merged commit f1f81b8 into xarray-contrib:main Jul 7, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

read_pam_sidecar misaligns category_names when the RAT Value column is sparse or not zero-based

2 participants