Skip to content

check for max dimensions#113

Open
Jaapel wants to merge 13 commits intoisce-framework:mainfrom
Jaapel:max_dimension_check
Open

check for max dimensions#113
Jaapel wants to merge 13 commits intoisce-framework:mainfrom
Jaapel:max_dimension_check

Conversation

@Jaapel
Copy link

@Jaapel Jaapel commented Dec 1, 2025

Fixes #111 .
Tests use unittest.mock.MagicMock to avoid creating large arrays during testing.

@Jaapel Jaapel requested a review from gmgunter as a code owner December 1, 2025 14:37
@Jaapel
Copy link
Author

Jaapel commented Dec 1, 2025

@mirzaees let me know what you think!

Comment on lines 108 to 113
if isinstance(tile_overlap, int):
x_overlap = y_overlap = tile_overlap
elif isinstance(tile_overlap, tuple):
# cannot unpack, tile_overlap shape is checked later
x_overlap = tile_overlap[0]
y_overlap = tile_overlap[1]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if isinstance(tile_overlap, int):
x_overlap = y_overlap = tile_overlap
elif isinstance(tile_overlap, tuple):
# cannot unpack, tile_overlap shape is checked later
x_overlap = tile_overlap[0]
y_overlap = tile_overlap[1]
if isinstance(tile_overlap, int):
y_overlap = x_overlap = tile_overlap
elif isinstance(tile_overlap, tuple):
# cannot unpack, tile_overlap shape is checked later
y_overlap = tile_overlap[0]
x_overlap = tile_overlap[1]

Copy link
Member

Choose a reason for hiding this comment

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

(though i'm seeing that elsewhere in the code it a little ambiguous if the tuple is (row, col) or (x, y). i'm the tuple is (row, column) )

Copy link
Author

Choose a reason for hiding this comment

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

Fixed this

Copy link
Author

Choose a reason for hiding this comment

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

The ntiles docs say row / col, so I may have to revert this: https://github.com/Jaapel/snaphu-py/blob/afe62434dfeddf9875dadf09784c412228f38d5c/src/snaphu/_unwrap.py#L213. Can you confirm?

tile_shapes_max = (
arr.shape[0] + 1 // ntiles[0] + x_overlap,
arr.shape[1] + 1 // ntiles[1] + y_overlap,
)
Copy link
Member

Choose a reason for hiding this comment

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

was this logic taken from somewhere in snaphu? i dont think it looks right... + 1 // ntiles[0] will always either be 1 or 0, and this would . maybe you want something like

Suggested change
)
# in case tile exceeds max array size
tile_height = math.ceil(arr.shape[0] / ntiles[0]) + y_overlap
tile_width = math.ceil(arr.shape[1] / ntiles[1]) + x_overlap
tile_shapes_max = (tile_height, tile_width)

but it could be good to check the C code for the actual max shape

Copy link
Author

Choose a reason for hiding this comment

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

I have checked the C code now

Copy link
Author

Choose a reason for hiding this comment

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

I forget the ( in the answer you've reviewed. I also updated the tests to be more precise.

@Jaapel Jaapel requested a review from scottstanie December 1, 2025 16:19
Copy link
Member

@gmgunter gmgunter left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to improve the codebase. Nice use of mocking in the unit test.

# a single tile is input for snaphu
for size in arr.shape:
if size > LARGESHORT:
msg = f"dataset {name} too large for snaphu, shape: {arr.shape}"
Copy link
Member

Choose a reason for hiding this comment

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

Let's add some suggestions here to help users work around this.

Suggested change
msg = f"dataset {name} too large for snaphu, shape: {arr.shape}"
msg = (
f"{name} dataset with shape {arr.shape} exceeds max dimensions"
" supported by SNAPHU. Consider using tiling and disabling the"
" regrow_conncomps and single_tile_reoptimize options"
)

Jaapel and others added 10 commits December 2, 2025 09:36
Co-authored-by: Geoffrey Gunter <12984092+gmgunter@users.noreply.github.com>
Co-authored-by: Geoffrey Gunter <12984092+gmgunter@users.noreply.github.com>
Co-authored-by: Geoffrey Gunter <12984092+gmgunter@users.noreply.github.com>
Co-authored-by: Geoffrey Gunter <12984092+gmgunter@users.noreply.github.com>
Co-authored-by: Geoffrey Gunter <12984092+gmgunter@users.noreply.github.com>
Co-authored-by: Geoffrey Gunter <12984092+gmgunter@users.noreply.github.com>
Co-authored-by: Geoffrey Gunter <12984092+gmgunter@users.noreply.github.com>
Co-authored-by: Geoffrey Gunter <12984092+gmgunter@users.noreply.github.com>
Co-authored-by: Geoffrey Gunter <12984092+gmgunter@users.noreply.github.com>
@Jaapel
Copy link
Author

Jaapel commented Dec 2, 2025

Thanks for the extended review! I implemented all your suggestions @gmgunter .
@scottstanie I implemented your changes, but based on the unwrap docs, ntiles is a (row, col) tuple. So I guess I need to rename y to row and x to col?

@mirzaees
Copy link
Contributor

@scottstanie and @gmgunter please approve this PR if it is final

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.

RuntimeError: one or more interferogram dimensions too large when tiling and regrowing large interferogram

4 participants