-
-
Notifications
You must be signed in to change notification settings - Fork 647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
experiment with round to nearest for image size change #491
Comments
One option to consider is whether using floating point values for scale factors is a good idea. Alternatives include:
Personally I think if you stick with a scale factor you need to use appropriate rounding. Alternatively explicitly specify width and height is a good idea too. Keep in mind not all resizing is integral.. Not sure if you have captured that in the API but it might also be worth considering.. |
Also wrt your assertion regarding preventing fractional pixels, how is round any different? It still produce integral values, just ones that are probably what the user was expecting, rather than truncation. |
I use vips for medical imaging, I need to be able to resize images so that matching pixels line up. For example, if I have a pair of slices from CT scans taken at different resolutions, I need to be able to upsize the smaller one using the ratio of the resolutions so that the two images can be overlaid. Resize therefore must take a float scale factor. On fractional pixels: for example, suppose you have a 100 pixel across image and you scale by 0.995. Now the output size will be 99.5, if you round-to-nearest, that's back to 100 again. The right-most column of pixels in the output will only be half a pixel wide. Probably the simplest fix is just to add another column of pixels to the right edge of the input, but I suppose you could also (arguably) make it 50% transparent. Anyway, strict round down means you don't have to deal with these problems. A related issue is corner vs. centre convention. vips is corner, but with extra parameters which let you give an input offset to produce centre if you wish. I'm a bit unclear if that might need revisiting too. |
seems to all work, test it a bit more see https://github.com/jcupitt/libvips/issues/491
I've had a go in this branch: https://github.com/jcupitt/libvips/tree/try-round-to-nearest It seems to pass the basic tests, I'll try it on some of my workloads. Any other testing would be great! @felixbuenemann, you should be able to remove your 0.1 hack, I think, though it's also fine to leave it in. |
@lovell, I meant to ask you about this too, do you have any size rounding hacks in sharp? This could make them removable. |
Some previous versions of sharp contained the |
@jcupitt Thanks, I did a quick check and it works fine for the common cases. I found a weird edge case though, where the result was unexpected: # 100x1 black image
original = Vips::Image.black(100, 1)
# resize to 50x1
scale = 50 / original.width.to_f
=> 0.5
resized = original.resize(scale)
resized.width
=> 1 # expected 50
resized.height
=> 1 # ok So instead of rounding the height from 0.5 to 1 pixel it instead resized the width to 1 px. Note that in vips 8.3.3 this fails with:
|
@jcupitt Do the C++ bindings for shrink/reduce/v/h need updating on the |
Those changes should be invisible I think @lovell, they are not exposed in the API, except as names in prototype declarations. |
Oh argh, you're right @lovell, they do need updating, sorry about that. |
@felixbuenemann I tried in python and it seems to work. I see:
I'll have a go with ruby-vips. |
Yeah, this somehow behaves differently on ruby-vips vs. pyvips8. I tried in python and am getting the same error as you posted above. |
@jcupitt Thanks for updating the C++ bindings - this change looks good from sharp's perspective. |
Yes, looks like a ruby-vips problem. I've fixed a few things and pushed 1.0.3. I now see:
Which I think looks correct. |
@jcupitt Yeah, I'm getting the same error now in ruby-vips. However in my calculations |
I think that's just float arithmetic, it's never exact. 0.5 is not precisely 0.5 in binary. I experimented:
So the first line is rounding down, the second is rounding up, and they differ in just 0.0000000000000001. |
@jcupitt 0.5 can most definitely be represented accurately in IEEE float and double data types. Floating point numbers are in base 2, which applies to the decimal part. So, a floating point number consists of a sequence of sums of 2**-n where for all n is the bit position - so, 2**-1 = 0.5, 2**-2 = 0.25, therefore 0.25, 0.5 and 0.75 could all be represented accurately as floating point numbers. |
@felixbuenemann did you look at the documentation for round? for x.y, where y < 0.5, round(x.y) -> x, otherwise round(x.y) -> x+1 |
@ioquatix Oh, true, I was thinking of 0.1 perhaps. Anyway, that 0.5 argument to resize goes through a long chain of processing, you would expect it to drift a little. |
@jcupitt The problem I have with the rounding differences is this: If I don't know how the resize internal rounding differs from normal rounding, then how should I calculate the correct scale value? |
@felixbuenemann because it's now round-to-nearest, you have a 0.5 margin of error, so it doesn't matter. Just do: scale = target_width.to_f / original_width And it'll be correct. I think your corner-case is a different issue: what should we do if an axis length rounds down to zero? We could add a new rules preventing this, I suppose, but it would need a little thought. If you make a 100 x 2 pixel black, your problem goes away:
|
Except it isn't correct, as the discussion above shows, that Your commit showed a similar issue: 1450 * 0.33
=> 478.5
x = Vips::Image.black(1450, 1450)
y = x.resize(0.33)
y.width
=> 478 # expected 479 So apparently Regarding the round to zero case: As long as the rounding is predictable, I think the exception is ok. |
I'll see if I can improve the rounding behaviour, maybe there's a little more precision to find. Your example would work if you calculated the scale factor:
So I'm not sure this is an issue in practice. You can't expect exact results with float arithmetic. |
I actually traced it back to the C /* compile with
*
* gcc -Wall rint.c -lm
*
*/
#include <math.h>
#include <stdio.h>
int
main( int argc, char **argv )
{
double x = 0.5;
printf( "x = %g\n", x );
printf( "rint( x ) = %g\n", rint( x ) );
printf( "(int) (x + 0.5) = %d\n", (int) (x + 0.5) );
return( 0 );
} Output for me is:
So I guess vips is no worse than |
Ah, got it, it depends if the number is odd or even. 1.5 is not exactly representable as binary float, it's stored as 1.50000000000001. So Anyway, floating point number madness, don't worry about it, IMO. |
Where are you using If you read the documentation for |
Just FYI, the second answer of this is correct: http://mathematica.stackexchange.com/questions/2116/why-round-to-even-integers |
So, just to clarify, you don't want to use |
That's interesting, I didn't know about Banker's Rounding, thanks! I've changed it to
|
OK, merged to master, this will be in 8.4. Thank you everyone. |
@jcupitt Is it possible to get this fix in a dot release? and/or curious about the 8.4 release date. Thanks for a great library. |
This is a change in behavior rather than a simple bugfix, so I don't think it belongs in a patch release. Then again I'm no maintainer, so John might have a different point of view. @aarti Is there a particular reason you can't use the current master? |
It's ok, will wait for 8.4, using a workaround for now. |
Should we start a new issue about the option of giving explicit integers when you know the exact width or height you want? It is pretty awkward to use this on the command line tool when you, for example, want to down-sample a bunch of images of different sizes to the same width. |
@Meekohi I would suggest a new issue, this issue is closed and deals with rounding issues, while your concern is mostly with usability. |
Sure, please open an issue. I would use |
vips always does int() on image dimensions for a size change. The aim is to prevent fractional pixels at image edges. If you want to size to a specific number of pixels, you need to add a small positive offset to the target size to prevent rounding errors producing a smaller image than you wanted.
This seems ugly. Investigate switching to round to nearest rather than strict round down. This will affect at least resize, affine, reduce, reduceh, reducev, shrink.
See https://github.com/jcupitt/libvips/pull/490
The text was updated successfully, but these errors were encountered: