bugGNU Octave - Bugs: bug #46390, image package - im2bw interprets...

 
 

bug #46390: image package - im2bw interprets threshold slightly different than Matlab

Submitter:  Hartmut <hardy>
Submitted:  Fri 06 Nov 2015 08:00:11 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * other Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 20 Nov 2015 11:17:13 AM UTC, comment #13: 


> Attached changeset with tests


Thank you. I have pushed your extra tests.

Carnë Draug <carandraug>
Group Member
Wed 18 Nov 2015 09:00:08 PM UTC, comment #12: 

Attached changeset with tests


(file #35496)

Avinoam Kalma <avinoam>
Group Member
Thu 12 Nov 2015 09:19:29 PM UTC, comment #11: 

Pushed with cast and floor for the reasons on the comment below

  http://hg.code.sf.net/p/octave/image/rev/6403613d984c

and added a simple test for int16 image

  http://hg.code.sf.net/p/octave/image/rev/9d79984e9b3e

However, I would still appreciate more tests to this function.

Carnë Draug <carandraug>
Group Member
Thu 12 Nov 2015 09:08:09 PM UTC, comment #10: 

Well, I think it is good to make it explicit that we do want it cast and
floored (and not only cast(), remember negative values).

Interestingly, I do get the same results as you:


I = uint8([0:255]);

## cast and floor
t = cputime (); for i=0:10000, bw = im2bw(I, i/100000); endfor, cputime() -t

ans =  3.7840

## no cast and no floor
t = cputime (); for i=0:10000, bw = im2bw(I, i/100000); endfor, cputime() -t

ans =  2.9200


However, when I use random and larger matrices, that doesn't happen anymore:


I = randi (255, 1000, "uint8");

## cast and floor
t = cputime (); for i=0:10000, bw = im2bw(I, i/100000); endfor, cputime() -t
ans =  13.024

## no cast and no floor
t = cputime (); for i=0:10000, bw = im2bw(I, i/100000); endfor, cputime() -t
ans =  15.292



For curiosity, I also timed the actual comparison step and it is faster
to make the cast explicitely:


a = randi (255, 1000, "uint8");

t_d = 190;
t_u = uint8 (t_d);

for i=1:100, b = a > t_d; endfor, t = cputime (); for i=1:1000, b = a > t_d; endfor, cputime () -t
ans =  1.2880

for i=1:100, b = a > t_u; endfor, t = cputime (); for i=1:1000, b = a > t_u; endfor, cputime () -t
ans =  0.90400

for i=1:100, b = a > t_u; endfor, t = cputime (); for i=1:1000, b = a > cast (t_u, "uint8"); endfor, cputime () -t

for i=1:100, b = a > t_u; endfor, t = cputime (); for i=1:1000, b = a > cast (t_d, "uint8"); endfor, cputime () -t
ans =  0.99200

for i=1:100, b = a > t_u; endfor, t = cputime (); for i=1:1000, b = a > cast (floor(t_d), "uint8"); endfor, cputime () -t
ans =  1.0280


Carnë Draug <carandraug>
Group Member
Thu 12 Nov 2015 08:15:58 PM UTC, comment #9: 

Thanks for fixing the cset. Please push the attached cset,
but remove me from the copyright line. Thanks :-)

Next week I hope to add more tests.

Just a question: I have compare to execution times for this
version and for version with


thres = (thres * I_range) + I_min


i.e. leaving the thresh in double, without changing image type.
using the following script


I = uint8([0:255]);

S = 0;
for i=0:10000
    bw = im2bw(I, i/100000);
    s = sum(bw);
    S = S+s;
end


I got the same results but 20% faster.
Any reason for doing the cast & floor

Avinoam Kalma <avinoam>
Group Member
Thu 12 Nov 2015 02:50:09 PM UTC, comment #8: 

Ah, yes you are right.  For some report, on the original report I read that Matlab was reporting 24 instead of 26. My changes actually made it worse.  Should I push the attached cset then?

Also, since it seems you have Matlab access, I would be grateful if you could had a few more tests, with int8 and int16 input as well.

(file #35440)

Carnë Draug <carandraug>
Group Member
Wed 11 Nov 2015 06:58:57 AM UTC, comment #7: 

Thanks for improving the patch.

There are still two problems with the fix:
1. The addition of 1 to the range is wrong
2. ceil should be replaced by floor.

see for example:


I = uint8([0:255]);

S = 0;
for i=0:10
    bw = im2bw(I, i/10);
    s = sum(bw);
    S = S+s;
end

disp(S);


Matlab (2010b) result is 1405, while Octave result is 1394.
We can add this small example as a BIST.

Avinoam Kalma <avinoam>
Group Member
Tue 10 Nov 2015 07:41:55 PM UTC, comment #6: 

Thank you guys for handling this.

On your last patch the computed range was off by just 1.  I pushed a slightly different version (it also removes string comparison for the class name):

  http://hg.code.sf.net/p/octave/image/rev/c4834a5278a4

Would be great if you could further the tests for this function, as it is, it's quite small.

Carnë Draug <carandraug>
Group Member
Mon 09 Nov 2015 08:10:41 PM UTC, comment #5: 

About the first point, yes you are right, a fixed patch is attached.

About the second point, if you want to make thresh in the same class as the image, you should use floor and casting, not round. If you will use round, you will get the wrong result again. But there
is no difference in the result, if the thresh is left double or being floored, so AFAIU there's no need to change the thresh from double (but maybe I'm wrong?)


(file #35428)

Avinoam Kalma <avinoam>
Group Member
Sun 08 Nov 2015 09:23:25 PM UTC, comment #4: 

In principle yes, but I still see some problems with it:

  • (intmax(cls) - imin) does NOT work. Give it a try with cls='int16' on the command line. Those numbers need to be transformed for example to class double before a big values like 65535 can be hold within a variable of this type at all.


  • The resulting class of thresh should NOT be double, as it is in your patch right now. But thresh needs to have the class of the input image (as it was in the original code).


Sorry, I dont' have enough time to further think about it now. I will try to come back to this at the end of the week.

Hartmut <hardy>
Sun 08 Nov 2015 08:52:06 PM UTC, comment #3: 

something like


cls = class(image);
imin = double(intmin(cls))
thres = thres*double(intmax(cls)-imin)+imin;


??

Avinoam Kalma <avinoam>
Group Member
Sun 08 Nov 2015 08:29:53 PM UTC, comment #2: 

The provided patch looks reasonable to me for input images of the classes uint8 and uint16. But I think it will produce wrong results with int16 images because intmin('int16') = -32768. But there should be an easy way to improve this patch, by additionally using the intmin value of the input class.

Hartmut <hardy>
Sun 08 Nov 2015 06:57:06 PM UTC, comment #1: 

As I understand the thresh, it seems to me that if the thresh is 0.9, it should be translated to 0.9 x 255 = 229.5, without
any rounding, so 230 and up will be included, as in Matlab.

See the attached diff.


(file #35417)

Avinoam Kalma <avinoam>
Group Member
Fri 06 Nov 2015 08:00:11 PM UTC, original submission:  

I observed this behavior with Octave 4.0.0 release and the image package release 2.4.1 (under Linux). Here is a little skript to show the behavior:


I = uint8([0:255]);
bw = im2bw(I, 0.9);
sum(bw)


If you let this skript run in Octave, the result is 25. This means the Octave version of im2bw decides that the values between 231 and 255 are above a threshold of 90%.

If you let this script run in Matlab (R2013b), the result is 26. This means the Matlab version of im2bw decides that the values between 230 and 255 are above a threshold of 90%.

This is (only) an incompatibility. The results of the Octave version of im2bw are also well usable. But they ARE different to the Matlab results, you always get a few less foreground pixels with Octave.

In my naive understanding, the Matlab result is "more correct". When I would be asked to divide the set [0 ... 255] into a chunk with 90% and another chunk with 10% of the numbers, then I would calculate 256*0.9 = 230.4. And then I would start to count 230 elements of this set, and end up with 0 ... 229. So the value 230 would be the first one of the remaining 10%, those values being 230...255. This is the Matlab result.

So I guess this issue is probably either about

  • how to round a float percentage to the next integer value, or about
  • starting to count at 0 or at 1, or about
  • taking 255 or 256 as maximum value of the uint8 numbers.



Hartmut <hardy>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #35496:  im2bw.m_tests.cs added by avinoam (1KiB - text/plain)
file #35440:  im2bw-bug46390.cset added by carandraug (3KiB - application/octet-stream - cset with Avinoam proposed changes)
file #35428:  im2bw2.diff added by avinoam (740B - application/octet-stream)
file #35417:  im2bw.diff added by avinoam (646B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by avinoam (Updated the item)
  • -email is unavailable- added by hardy (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-11-18 avinoam Attached File- Added im2bw.m_tests.cs, #35496
    2015-11-12 carandraug StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2015-11-12 carandraug StatusFixed Confirmed
        Open/ClosedClosed Open
    2015-11-12 carandraug Attached File- Added im2bw-bug46390.cset, #35440
    2015-11-10 carandraug StatusNone Fixed
        Open/ClosedOpen Closed
        Release4.0.0 other
    2015-11-09 avinoam Attached File- Added im2bw2.diff, #35428
    2015-11-08 avinoam Attached File- Added im2bw.diff, #35417

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code