Skip to content

fix: clear error on disjoint instance IDs in render_shapes/labels/points#661

Open
SAY-5 wants to merge 1 commit into
scverse:mainfrom
SAY-5:SAY-5/fix/shapes-disjoint-instance-ids
Open

fix: clear error on disjoint instance IDs in render_shapes/labels/points#661
SAY-5 wants to merge 1 commit into
scverse:mainfrom
SAY-5:SAY-5/fix/shapes-disjoint-instance-ids

Conversation

@SAY-5
Copy link
Copy Markdown
Contributor

@SAY-5 SAY-5 commented May 12, 2026

Closes #603.

When a table annotates the element (region key matches) but no instance_id values overlap, the call crashed with a bare KeyError: None from deep inside upstream spatialdata's join path. The traceback gave no indication that the instance ID sets were disjoint.

Added an explicit disjoint check before join_spatialelement_table: when at least one table row claims to annotate the element yet the instance_key values share nothing with the element's index, raise a ValueError naming the table, element, instance key, and the fix. Regression test in test_render_shapes.py.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 12, 2026

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.57%. Comparing base (7e6a607) to head (2e33801).

Files with missing lines Patch % Lines
src/spatialdata_plot/pl/render.py 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #661      +/-   ##
==========================================
+ Coverage   77.54%   77.57%   +0.02%     
==========================================
  Files          11       11              
  Lines        3572     3581       +9     
  Branches      838      840       +2     
==========================================
+ Hits         2770     2778       +8     
  Misses        480      480              
- Partials      322      323       +1     
Files with missing lines Coverage Δ
src/spatialdata_plot/pl/render.py 87.36% <88.88%> (+0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/spatialdata_plot/pl/render.py Outdated
if _saved_index_name is not None and _saved_index_name in _obs.columns:
_obs.index.name = None

# Guard against a disjoint instance_key (#603): when no table rows annotate any element
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Claude tends to be overly verbose, please remove this comment

Comment thread tests/pl/test_render_shapes.py Outdated


def test_render_shapes_disjoint_instance_ids_clear_error():
# Regression test for #603: when a table annotates the element (region key matches) but no
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is also overly verbose, reduce to one-liner

@timtreis
Copy link
Copy Markdown
Member

Hey @SAY-5, thanks for your contribution. Few issues with this PR:

Your new guard sits between the _obs.index.name = None workaround and the try/finally that restores it. If the guard raises, the sdata[table_name].obs.index.name is left mutated which is a problem.

Also, this fix only applies to shapes, the same behaviour can happen for labels and points, so we'd need the same fix there.

Comment thread tests/pl/test_render_shapes.py Outdated
# Regression test for #603: when a table annotates the element (region key matches) but no
# instance_id values overlap, the call used to crash with a bare `KeyError: None` from deep
# inside spatialdata's join path. Replace with a clear, actionable ValueError.
from shapely.geometry import Point
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't reimport inside the test

Comment thread src/spatialdata_plot/pl/render.py Outdated
# Guard against a disjoint instance_key (#603): when no table rows annotate any element
# row, the upstream join crashes with `KeyError: None` from deep inside pandas. Raise
# a clear, actionable ValueError before invoking the join.
_, _region_key, _instance_key = get_table_keys(sdata[table_name])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Operate on sdata_filt instead of sdata here

@SAY-5 SAY-5 force-pushed the SAY-5/fix/shapes-disjoint-instance-ids branch from 5a5900e to 95fc089 Compare May 12, 2026 17:02
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 12, 2026

Thanks for the review. Reworked in 95fc089:

  • Moved the disjoint-instance-ID guard so it runs before the obs.index.name workaround in _render_shapes, so a failed check can never leave the index name half-restored.
  • Extracted the check into a shared _check_instance_ids_overlap helper and applied it to _render_labels (vs the label IDs) and _render_points (vs the points index) as well, so the disjoint-IDs case raises the same clear ValueError instead of an opaque KeyError: None for all three render functions.
  • Added regression tests on the disjoint-IDs path for shapes, labels, and points; ruff check/ruff format clean.

Also retitled the PR to reflect the wider scope.

@SAY-5 SAY-5 changed the title fix: clear error on disjoint instance IDs in render_shapes fix: clear error on disjoint instance IDs in render_shapes/labels/points May 12, 2026
@timtreis
Copy link
Copy Markdown
Member

Hey @SAY-5, thanks for the update! I had also left some more comments in the code directly that aren't yet addressed, also, the branch has conflicts with main. Would be great if you could finalise the PR 👍 Thanks!

@SAY-5 SAY-5 force-pushed the SAY-5/fix/shapes-disjoint-instance-ids branch from 95fc089 to 65aed4d Compare May 12, 2026 19:23
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 12, 2026

Rebased onto current main (resolved a conflict in tests/pl/test_render_labels.py by keeping both the new instance_id==0 background guard test from #660 and this PR's disjoint-instance-ID regression test). ruff check and ruff format pass; the full non-plot test suite for test_render_shapes/labels/points (78 tests, including the 3 regression tests added here) passes locally. Ready for another look.

@SAY-5 SAY-5 force-pushed the SAY-5/fix/shapes-disjoint-instance-ids branch from 65aed4d to ebeb67a Compare May 12, 2026 20:53
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 12, 2026

Addressed the inline notes: trimmed the verbose comments to one line, dropped the redundant in-test shapely reimport, and switched the shapes guard to use sdata_filt[element].index. ruff check/format pass.

@SAY-5 SAY-5 force-pushed the SAY-5/fix/shapes-disjoint-instance-ids branch from ebeb67a to 2e33801 Compare May 12, 2026 23:51
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 12, 2026

Switched the overlap check to operate on sdata_filt and dropped the verbose comments. Rebased on main so there are no conflicts. New regression tests pass locally.

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.

render_shapes crashes with KeyError: None when table instance IDs don't match element

3 participants