Skip to content

Fix encoding of Long Primitive Point Index List in ANN - #467

Open
fedorov wants to merge 1 commit into
masterfrom
bug/ann_point_index_list
Open

fedorov wants to merge 1 commit into
masterfrom
bug/ann_point_index_list

Conversation

@fedorov

@fedorov fedorov commented Oct 6, 2026

Copy link
Copy Markdown
Member

Note on how this PR was produced: The code changes, tests and this description were generated by Claude Code (Anthropic's Claude Opus 5.5 model), working in a local clone of this repository at the request of @fedorov. They follow the analysis in #466.

Addresses #466.

Problem

For POLYGON and POLYLINE annotation groups, Long Primitive Point Index List (0066,0040) has to give the one-based index of the first point (coordinate tuple) of each annotation. highdicom instead encoded and decoded the index of the first coordinate value. For two 2D polygons where the first has 4 points, highdicom wrote [1, 9]; the standard requires [1, 5]. See #466 for the full analysis and the WG-26 clarification.

Changes

  • Encoding (AnnotationGroup.__init__): spans now counts points per annotation instead of values. The unused dimensionality variable is removed.
  • Decoding (AnnotationGroup.get_graphic_data()): goes through a new _get_point_offsets() helper, which:
    • checks that the list starts at 1 and is strictly increasing, and raises ValueError otherwise. Previously, invalid values silently produced wrong splits.
    • treats the values as point indices whenever they are valid point indices.
    • detects the legacy value-based encoding when the last value is not a valid point index but all values fit value-based offsets ((idx - 1) % d == 0 and idx - 1 < N * d). It then emits a UserWarning and divides by the stored dimensionality d, so existing files, including the 6,075 IDC pan_cancer_nuclei_seg_dicom series, still decode correctly.
    • raises ValueError if neither interpretation fits.
  • Removed _get_coordinate_index(). It was never called and also assumed value-based indices.
  • Tests: new TestAnnotationGroupPointIndexList covering:
    • the raw encoded LongPrimitivePointIndexList ([1, 5]) for 2D, 3D with common Z and explicit 3D, for both POLYGON and POLYLINE, followed by a decode check
    • decoding legacy lists ([1, 9], [1, 9], [1, 13]) with a warning
    • rejection of four kinds of invalid list
  • Docs: migration note in docs/release_notes.rst.

Known limitation

The detection uses heuristics. In a legacy value-based file, if every value also happens to be a valid point index, it is decoded as point-based, which is wrong for that file. This happens only when the last annotation contains more than half of all points (2D or common Z) or more than two-thirds (explicit 3D). Single-annotation groups are encoded [1] either way and are unaffected.

Testing

  • The new tests fail on unpatched master (13 failed) and pass with this change.
  • Full suite: 2047 passed, 153 skipped (skips are optional codecs/libraries not installed locally). flake8 is clean.
  • Decoded the real IDC file 1.2.826.0.1.3680043.10.511.3.61063851916899676364195955814616690 (legacy index list [1 31 57 97 117 155 199 243], 138 points). The patched reader emits the legacy warning and returns 8 polygons with [15, 13, 20, 10, 19, 22, 22, 17] points, the same result as the unpatched reader.

Follow-up outside this PR

  • dicom-microscopy-viewer / Slim probably use the same value-based interpretation (not verified). They should be updated at the same time, otherwise files written by highdicom after this change will display incorrectly.
  • IDC to decide whether to re-convert the existing polygon ANN series.

🤖 Generated with Claude Code

Long Primitive Point Index List (0066,0040) of POLYGON and POLYLINE
annotation groups was encoded and decoded as the one-based index of the
first coordinate value of each annotation in Point Coordinates Data,
rather than the index of the first point (coordinate tuple), as intended
by the standard (clarified on DICOM WG-26). For two 2D polygons where
the first has 4 points, highdicom wrote [1, 9] instead of [1, 5].

- Encode point indices in AnnotationGroup
- Decode point indices in get_graphic_data(), validating the values and
  detecting the legacy value-based encoding (with a warning) where the
  values cannot be valid point indices
- Replace unused _get_coordinate_index() helper
- Add tests asserting the raw encoded values, legacy decoding and
  rejection of invalid values
- Add migration note to release notes

Addresses #466

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant