Re: Suspicious code in interpolation code

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Suspicious code in interpolation code

Antoine Vandecreme
Hi all,

I was looking at the resizing code and I found this part suspicious:
https://github.com/fiji/imagej1/blob/master/ij/process/ShortProcessor.java#
L844
Lines 844 to 847:
the lowerLeft variable seems to be assigned the upperLeft value (and
upperLeft variable the lowerLeft value)
the same goes for lowerRight and upperRight.

To be clear here is what I would write:
                int upperLeft = pixels[offset]&0xffff;
                int upperRight = pixels[offset + 1]&0xffff;
                int lowerRight = pixels[offset + width + 1]&0xffff;
                int lowerLeft = pixels[offset + width]&0xffff;

This code is also present in others processors.

I don't think there is a case where it is really important but I wanted to
let you now just in case.

Regards,
Antoine Vandecreme

--
ImageJ mailing list: http://imagej.nih.gov/ij/list.html
Reply | Threaded
Open this post in threaded view
|

Re: Suspicious code in interpolation code

Michael Schmid
Hi Antoine,

interpolation in ImageJ works well, so I think the code is correct.

Concerning the text 'upper' and 'lower' in the code, it depends on how you see it:

Your version is correct for the image as it appears at the screen, but then one would also have to exchange 'upper' and 'lower' in line 850.

One might also see it such that the 'upper' y value is the higher one, and 'lower' refers to the lower y value.  Probably that was the idea when the code was written, and to me the expression in line 850 looks much more logical like this, then when exchanging 'upper' and 'lower' to match  the appearance on the screen.

Michael
________________________________________________________________
On Nov 18, 2013, at 20:43, Antoine Vandecreme wrote:

> Hi all,
>
> I was looking at the resizing code and I found this part suspicious:
> https://github.com/fiji/imagej1/blob/master/ij/process/ShortProcessor.java#
> L844
> Lines 844 to 847:
> the lowerLeft variable seems to be assigned the upperLeft value (and
> upperLeft variable the lowerLeft value)
> the same goes for lowerRight and upperRight.
>
> To be clear here is what I would write:
>                int upperLeft = pixels[offset]&0xffff;
>                int upperRight = pixels[offset + 1]&0xffff;
>                int lowerRight = pixels[offset + width + 1]&0xffff;
>                int lowerLeft = pixels[offset + width]&0xffff;
>
> This code is also present in others processors.
>
> I don't think there is a case where it is really important but I wanted to
> let you now just in case.
>
> Regards,
> Antoine Vandecreme

--
ImageJ mailing list: http://imagej.nih.gov/ij/list.html
Reply | Threaded
Open this post in threaded view
|

Re: Suspicious code in interpolation code

Antoine Vandecreme
In reply to this post by Antoine Vandecreme
Hi Michael,
Thanks for the clarification. It makes sense.
Antoine
>Hi Antoine,
>
>interpolation in ImageJ works well, so I think the code is correct.
>
>Concerning the text 'upper' and 'lower' in the code, it depends on how
you see it:
>
>Your version is correct for the image as it appears at the screen, but
then one would also have to exchange 'upper' and 'lower' in line 850.
>
>One might also see it such that the 'upper' y value is the higher one, and
'lower' refers to the lower y value.  Probably that was the idea when the
code was written, and to me the expression in line 850 looks much more
logical like this, then when exchanging 'upper' and 'lower' to match  the
appearance on the screen.
>
>Michael
>________________________________________________________________
>On Nov 18, 2013, at 20:43, Antoine Vandecreme wrote:
>
>> Hi all,
>>
>> I was looking at the resizing code and I found this part suspicious:
>>
https://github.com/fiji/imagej1/blob/master/ij/process/ShortProcessor.java#[
1]

>> L844
>> Lines 844 to 847:
>> the lowerLeft variable seems to be assigned the upperLeft value (and
>> upperLeft variable the lowerLeft value)
>> the same goes for lowerRight and upperRight.
>>
>> To be clear here is what I would write:
>>                int upperLeft = pixels[offset]&0xffff;
>>                int upperRight = pixels[offset + 1]&0xffff;
>>                int lowerRight = pixels[offset + width + 1]&0xffff;
>>                int lowerLeft = pixels[offset + width]&0xffff;
>>
>> This code is also present in others processors.
>>
>> I don't think there is a case where it is really important but I wanted to
>> let you now just in case.
>>
>> Regards,
>> Antoine Vandecreme

--------
[1]
https://github.com/fiji/imagej1/blob/master/ij/process/ShortProcessor.java#

--
ImageJ mailing list: http://imagej.nih.gov/ij/list.html