Add explicit error handling in _make_tiles.py

[timtreis]

Summary

• Adds upfront parameter validation with clear KeyError/ValueError messages to make_tiles(), make_tiles_from_spots(), _get_largest_scale_dimensions(), and _get_spot_coordinates()
• Validates that image/spots keys exist in sdata, tile_size has positive values, and min_tissue_fraction is in [0, 1]
• Adds 5 test cases covering all new error conditions

Closes #1086

Test plan

• test_make_tiles_invalid_image_key — bad image key raises KeyError
• test_make_tiles_invalid_tile_size — non-positive tile size raises ValueError
• test_make_tiles_invalid_min_tissue_fraction — out-of-range fraction raises ValueError
• test_make_tiles_from_spots_invalid_spots_key — bad spots key raises KeyError
• test_make_tiles_from_spots_invalid_image_key — bad image key raises KeyError

🤖 Generated with Claude Code

[selmanozleyen]

these can be one liners with a common check function. also easier to read

[timtreis]

I prefer not to code-golf at the sake of readability. Generally I find the asser thing, text pattern ugly

[selmanozleyen]

It's not just code golfing, it makes sense as a function itself. When the error message or access pattern changes you could change it from the function instead modifying it everywhere. Also I find it more readable this way. I can just skip reading those functions as they aren't part of the core algo. These kinds of trivial checks bloat the core function.

Idk something like this

def check_element_exists(
    sdata,
    key,
    element_type,
):
    store = getattr(sdata, element_type)
    if key not in store:
        available = list(store.keys())
        raise KeyError(
            f"Key {key!r} not found in sdata.{element_type}. "
            f"Available keys: {available}"
        )

[selmanozleyen]

I am wondering if we should just let it error from spatialdata? Like the error message would be almost the same

[timtreis]

I am wondering if we should just let it error from spatialdata? Like the error message would be almost the same

I see the rational but dislike it in this case because sdata core has quite a few error messages that a biologist probably wouldn't intuitively understand and they also usually don't proactively offer help by f.e. giving the valid keys. There's also quite a few empty asserts which I find quite problematic.

When the error message or access pattern changes you could change it from the function instead modifying it everywhere. Also I find it more readable this way. I can just skip reading those functions as they aren't part of the core algo. These kinds of trivial checks bloat the core function.

I do see that point, but I think then it should be a separate PR where we do this across the entire codebase. wdyt?

[selmanozleyen]

Sure it can be a separate PR but isn't this Pr already small. Why not start here?

Update: sorry was in the bus so the comment was incomplete bc of that

[selmanozleyen]

but does this get rid of the try except block as written in the issue linked?

[codecov]

Codecov Report

❌ Patch coverage is 6.66667% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.96%. Comparing base (95d8b18) to head (4c230be).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/squidpy/experimental/im/_make_tiles.py	6.66%	7 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1130      +/-   ##
==========================================
- Coverage   73.10%   72.96%   -0.14%     
==========================================
  Files          38       38              
  Lines        6473     6484      +11     
  Branches     1144     1151       +7     
==========================================
- Hits         4732     4731       -1     
- Misses       1265     1270       +5     
- Partials      476      483       +7     

Files with missing lines	Coverage Δ
src/squidpy/experimental/im/_make_tiles.py	67.56% <6.66%> (-2.12%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[timtreis]

but does this get rid of the try except block as written in the issue linked?

True, we're validating beforehand, so the function should never crash and burn.

[selmanozleyen]

sorry but I stand by the comments I made earlier.

From the other codes I have read, I find these changes reasonable to expect.

For example see:
https://github.com/scverse/annbatch/blob/d47240f883f64b86524d6bc32731ce3e15be2984/src/annbatch/utils.py#L114

I don't think this is overengineering or anything I think it is a necessity

[timtreis]

sorry but I stand by the comments I made earlier.

From the other codes I have read, I find these changes reasonable to expect.

For example see: https://github.com/scverse/annbatch/blob/d47240f883f64b86524d6bc32731ce3e15be2984/src/annbatch/utils.py#L114

I don't think this is overengineering or anything I think it is a necessity

I just think it's suboptimal to do one function here and then others in a separate PR. If we do something like that, it should be across the entire package. I can just do that in this PR and rename it, but I'd rather not sneak in such a major refactor under "added error handling"

[selmanozleyen]

I just think it's suboptimal to do one function here and then others in a separate PR. If we do something like that, it should be across the entire package. I can just do that in this PR and rename it, but I'd rather not sneak in such a major refactor under "added error handling"

ok if you say so. I just feel like there are already lot's one can refactor we might as well start when we add new code but if you think you can do the PR then sure.

[timtreis]

discussed: have separate PR for type checking refactor