Raise warning and downsample if data given to _image.resample is too large#19368
Conversation
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
We can accurately downsample so I don't think we should error here. An aliased downsampled image is still zoomable, at which point the aliasing will go away. If the image is above 2**23 we should just subsample by a factor of 2 and warn.
👍 - for subsampling, would you recommend just doing |
|
I think we want |
|
Yep, my comment was modulo finding the right stride, but just wanted to double check that striding was the correct approach. |
|
The right approach is open to debate, in that you could anti-alias the data before hitting it with a stride. But for such a massive subsample, I think a simple stride is better than an unspecified overflow/wrap! |
dstansby
changed the title
dstansby
force-pushed
the
resample-error
branch
3 times, most recently
from
5f1773c to
1fd6269
Compare
dstansby
force-pushed
the
resample-error
branch
2 times, most recently
from
40b5664 to
a81575c
Compare
|
Why do we bother to downsample at all? We cannot display more than N pixels and should refuse to accept more (In the face of ambiguity refuse the temptation to guess). If a user has more data, it's their responsibility and competence to reduce them. |
|
I'm happy to go either way - perhaps worth discussing on a dev call and coming back with a decision? |
|
We almost always resample so I don't see the problem with reducing the data before trying the final resample step. This will be aliased, but a max-sized image will also be aliased by the final downsample, so I doubt it would be detectable. |
|
I am 👍🏻 in favor of this. |
|
As discussed on the call, we should go ahead and subsample and warn, which I think is what this PR does. |
|
👍 - I still need to add a test for resampling the columns that's similar to the existing one for rows |
dstansby
force-pushed
the
resample-error
branch
2 times, most recently
from
60a3373 to
cecd991
Compare
|
This should be good for review now. Tests and an API note added. |
dstansby
force-pushed
the
resample-error
branch
from
d38e82c to
525cd0c
Compare
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
Sorry to let this languish. I think this looks good.
| if data.shape[1] > 2**23: | ||
| warnings.warn(msg.format(n='2**23 rows')) | ||
| step = int(np.ceil(data.shape[1] / 2**23)) | ||
| data = data[:, ::step] | ||
| transform = Affine2D().scale(step, 1) + transform | ||
| if data.shape[0] > 2**24: | ||
| warnings.warn(msg.format(n='2**24 columns')) | ||
| step = int(np.ceil(data.shape[0] / 2**24)) | ||
| data = data[::step, :] | ||
| transform = Affine2D().scale(1, step) + transform |
There was a problem hiding this comment.
0 is the rows, 1 is the columns; you have the message swapped. I don't know if the limits should be swapped as well.
There was a problem hiding this comment.
Thanks - I checked the limits and they are correct, just the message that needed changing.
dstansby
force-pushed
the
resample-error
branch
from
525cd0c to
a9d28f3
Compare
|
Needs rebase for docs to work. |
tacaswell
force-pushed
the
resample-error
branch
from
a9d28f3 to
468b72c
Compare
|
power-cycled to pick up the CI config from main. |
5 participants
Fixes #19276. This still needs: