bugGNU Octave - Bugs: bug #55059, [octave forge] (image) Failing...

 
 

bug #55059: [octave forge] (image) Failing unit test for grayslice

Submitter:  Avinoam Kalma <avinoam>
Submitted:  Mon 19 Nov 2018 09:12:35 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  In Progress Assigned to:  None
Originator Name:  Avinoam Open/Closed:  * Open
Release:  * other Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 17 Jan 2021 07:21:53 PM UTC, comment #20: 

I have gone through the whole discussion in this bug report, again. There seemed to be one remaining issue with our grayslice function (mentioned in comment #14): float input images with values bigger than one. But when double checking this, it turned out the our current grayslice function behaves perfectly compatible in all those mentioned test codes. The reason why my test codes (still) give different results on Octave compared to Matlab is the following. This example vector

   vec = [ uint8(1) : uint8(5) ]

is of class uint8 in Matlab, but of class double in Octave. That's what cheated me into beleaving that there is even more wrong with our grayslice function.

I will file a new bug report for core Octave with this above behavior of the colon operator.

But this bug report here about grayslice.m in the image package can now be closed as FIXED, I think.

Hartmut <hardy>
Sun 17 Jan 2021 07:00:11 PM UTC, comment #19: 

I have now pushed the patch from comment #18 to the image repo: http://hg.code.sf.net/p/octave/image/rev/66f7a27fbfc8

Hartmut <hardy>
Tue 29 Dec 2020 03:50:21 PM UTC, comment #18: 

I attached a PATCH (V1) including my proposed changes from comment #16. Please review.

(file #50586)

Hartmut <hardy>
Mon 28 Dec 2020 09:23:00 PM UTC, comment #17: 

Looks OK, please make a patch. Thanks :-)

Avinoam Kalma <avinoam>
Group Member
Mon 28 Dec 2020 08:42:34 PM UTC, comment #16: 

When I understand my own (2 years old) comment #15 right, then the only missing bit to close this issue is the output of our grayslice function when feeding it with (signed) int16 signed integer inputs.

Regarding this missing bit, I just found the following information in the Matlab help:

Note:
Before thresholding an image of data type int16, the grayslice function converts the image to uint16 by adding 32,768 to each pixel. Consider this additive offset when specifying thresholds for input images of data type int16.

(source: https://de.mathworks.com/help/images/ref/grayslice.html)

So I suggest that we do the same in our Octave vesion of grayslice, to finally fix this issue.

An easy way to do this would be to add the following line somewhere before the "final" call to the "lookup" function (line 84 in our current repo version).

It turns out, we need to add this also before the creation of the v vector from a scalar n (line 56 in current repo version), because this uses the class if the image. So a good position would be just before this line 56:


  if isa (I, "int16")
  ## Convert int16 images to uint16, because that is what Matlab does.
    I = im2uint16 (I);
  endif

  if (isscalar (n) && n >= 1)  # currently existing code


As drawback I can only see the enlarged (double) memory consumption, but we would improve Matlab compatibility (and then also have their memory consumption).

When I apply the above mention code change, then

  • all tests still pass (except for the one xtest)
  • The very first test code in comment #15 (the one with int16 checks) finally gives the same result as Matlab.
  • The "1. test script" from comment #15 (the one using a scalar parameter) gives only Matlab compatible results.
  • The "2. test script" from comment #15 (the one with a vector parameter) also gives always compatible outputs.


So all looks fine to me.

Please let me know if you see any drawbacks with the suggested code change. Otherwise I will prepare a patch with this code change and some useful new tests.

Hartmut <hardy>
Mon 10 Dec 2018 09:23:06 PM UTC, comment #15: 

I have changed my test scripts (from comment #9 and comment #13) to properly use input images of all possible classes.

The result in short:

  • All outputs are now Matlab COMPATIBLE (using the current grayslice from the repository), EXCEPT for input images of class INT16 (signed integer):



 im = int16([intmin('int16'):intmax('int16')]);
 out = grayslice(im, [5 6]);
 first1_octave = im(find(out)(1))       % currently = 5
 first1_matlab = intmin('int16') + 5    %  = -32763


Conclusion: It seems to me that our Octave version is currently referencing signed integer values (like int16) to the zero value, but Matlab seems to reference them to the intmin value (of -32768). @Carne: Do you have an idea how to change our Octave version to fix this (i.e. the above test case)?

In the following I will (only) copy the full test scripts and their Matlab results for completeness and future reference:

1. test script with a scalar "n" parameter:


klassen = {'uint8', 'uint16', 'int16', 'single', 'double'};
for num = 1:length(klassen)
    disp('----------');
    klasse = klassen{num}
    if any(strcmp(klasse, {'uint8', 'int8', 'uint16', 'int16'}))
        vec = [intmin(klasse): intmax(klasse)];
        vec = cast(vec, klasse);
    else
        vec = [0:0.001:1];
        vec = cast(vec, klasse);
    end
    min_max = [min(vec), max(vec)]

    erg05 = grayslice(vec, [0.5]);
    first1_ind = find(erg05);
    first1_erg05 = vec(first1_ind(1))

    erg5 = grayslice(vec, [5]);
    first1_ind = find(erg5);
    first1_erg5 = vec(first1_ind(1))

    int5 = uint8(5);
    ergint5 = grayslice(vec, [int5]);
    first1_ind = find(ergint5);
    first1_ergint5 = vec(first1_ind(1))
end

disp('-------');
disp('Bonus: (uint8 0..100)');
vec = uint8([0:100]);
erg5 = grayslice(vec, 5);
first1_ind = find(erg5);
first1_erg5 = vec(first1_ind(1))

disp('Bonus: (double 0..100)');
vec = double(vec);
erg5 = grayslice(vec, 5);
first1_ind = find(erg5);
first1_erg5 = vec(first1_ind(1))


gives the following Matlab output:


 ----------
 klasse =
     'uint8'
 min_max =
   1x2 uint8 row vector
      0   255
 first1_erg05 =
   uint8
    1
 first1_erg5 =
   uint8
    51
 first1_ergint5 =
   uint8
    51
 ----------
 klasse =
     'uint16'
 min_max =
   1x2 uint16 row vector
        0   65535
 first1_erg05 =
   uint16
    1
 first1_erg5 =
   uint16
    13107
 first1_ergint5 =
   uint16
    13107
 ----------
 klasse =
     'int16'
 min_max =
   1x2 int16 row vector
    -32768    32767
 first1_erg05 =
   int16
    -32767
 first1_erg5 =
   int16
    -19661
 first1_ergint5 =
   int16
    -19661
 ----------
 klasse =
     'single'
 min_max =
   1x2 single row vector
      0     1
 first1_erg05 =
   single
     0.5000
 first1_erg5 =
   single
     0.2000
 first1_ergint5 =
   single
     0.2000
 ----------
 klasse =
     'double'
 min_max =
      0     1
 first1_erg05 =
     0.5000
 first1_erg5 =
     0.2000
 first1_ergint5 =
     0.2000
 -------
 Bonus: (uint8 0..100)
 first1_erg5 =
   uint8
    51
 Bonus: (double 0..100)
 first1_erg5 =
      1


2. test script with a vector "v" parameter:


klassen = {'uint8', 'uint16', 'int16', 'single', 'double'};
for num = 1:length(klassen)
    disp('----------');
    klasse = klassen{num}
    if any(strcmp(klasse, {'uint8', 'int8', 'uint16', 'int16'}))
        vec = [intmin(klasse): intmax(klasse)];
        vec = cast(vec, klasse);
    else
        vec = [0:0.001:1];
        vec = cast(vec, klasse);
    end
    min_max = [min(vec), max(vec)]

    erg0506 = grayslice(vec, [0.5 0.6]);
    first1_ind = find(erg0506);
    first1_erg0506 = vec(first1_ind(1))

    erg56 = grayslice(vec, [5 6]);
    first1_ind = find(erg56);
    first1_erg56 = vec(first1_ind(1))

    int56 = uint8([5 6]);
    ergint56 = grayslice(vec, int56);
    first1_ind = find(ergint56);
    first1_ergint56 = vec(first1_ind(1))
end

disp('-------');
disp('Bonus: (uint8 0..100)');
vec = uint8([0:100]);
erg5 = grayslice(vec, [5 6]);
first1_ind = find(erg5);
first1_erg5 = vec(first1_ind(1))

disp('Bonus: (double 0..100)');
vec = double(vec);
erg5 = grayslice(vec, [5 6]);
first1_ind = find(erg5);
first1_erg5 = vec(first1_ind(1))


gives the following Matlab result:


 ----------
 klasse =
     'uint8'
 min_max =
   1x2 uint8 row vector
      0   255
 first1_erg0506 =
   uint8
    1
 first1_erg56 =
   uint8
    5
 first1_ergint56 =
   uint8
    5
 ----------
 klasse =
     'uint16'
 min_max =
   1x2 uint16 row vector
        0   65535
 first1_erg0506 =
   uint16
    1
 first1_erg56 =
   uint16
    5
 first1_ergint56 =
   uint16
    5
 ----------
 klasse =
     'int16'
 min_max =
   1x2 int16 row vector
    -32768    32767
 first1_erg0506 =
   int16
    -32767
 first1_erg56 =
   int16
    -32763
 first1_ergint56 =
   int16
    -32763
 ----------
 klasse =
     'single'
 min_max =
   1x2 single row vector
      0     1
 first1_erg0506 =
   single
     0.5000
 first1_erg56 =
   single
      1
 first1_ergint56 =
   single
      1
 ----------
 klasse =
     'double'
 min_max =
      0     1
 first1_erg0506 =
     0.5000
 first1_erg56 =
      1
 first1_ergint56 =
      1
 -------
 Bonus: (uint8 0..100)
 first1_erg5 =
   uint8
    5
 Bonus: (double 0..100)
 first1_erg5 =
      5


Hartmut <hardy>
Fri 07 Dec 2018 11:06:43 AM UTC, comment #14: 

Currently all the results of the Octave grayslice function (in the repo) are Matlab compatible when running my test code from comment #13 (using a vector v for slicing).

My test code from comment #9 is giving different results on Matlab as on Octave currently. But I just realised that this difference is only for FLOAT IMAGES with VALUES BIGGER THAN 1. Because I unintentionally forgot to imcast the integer input images in these test codes. Currently ALL input images are of class double (because counter-intuitivly the output value of the intmin function is of class double).

Conclusion:

  • The non-compatible results from my current test code in comment #9 (wich were Carne's concern in comment #12) are not such a big thing. Because input images of class double but with most values bigger than 1 are no common use case anyway. The result is kind of undefined in my opinion.


  • I will try to supply Matlab results for more meaningful test codes next week.


Note: I haven't understood yet, why the one xtest in the current repo version is failing.

Question: Are all the tests in the function code currently Matlab compatible? (aka: Do I make things worse if I change the fucntion code and some of the tests start to fail?)

Hartmut <hardy>
Tue 04 Dec 2018 04:25:00 PM UTC, comment #13: 

I've just realised that my (Matlab) test results in comment #9 are not what I wanted to test. Their calls probably fall into the syntax "grayslice(I, n)", generating n slices.

Here is a new test code, that makes sure that the syntax "grayslice(I, v)" is called with a vector v:


klassen = {'uint8', 'uint16', 'int16', 'single', 'double'};
for num = 1:length(klassen)
    disp('----------');
    klasse = klassen{num}
    if any(strcmp(klasse, {'uint8', 'int8', 'uint16', 'int16'}))
        vec = [intmin(klasse): intmax(klasse)];
    else
        vec = [0:0.001:1];
        cast(vec, klasse);
    end
    min_max = [min(vec), max(vec)]

    erg0506 = grayslice(vec, [0.5 0.6]);
    first1_ind = find(erg0506);
    first1_erg0506 = vec(first1_ind(1))

    erg56 = grayslice(vec, [5 6]);
    first1_ind = find(erg56);
    first1_erg56 = vec(first1_ind(1))

    int56 = uint8([5 6]);
    ergint56 = grayslice(vec, int56);
    first1_ind = find(ergint56);
    first1_ergint56 = vec(first1_ind(1))
end


And here are the corresponding Matlab results:

 ----------
 klasse =
     'uint8'
 min_max =
   1x2 uint8 row vector
      0   255
 first1_erg0506 =
   uint8
    1
 first1_erg56 =
   uint8
    5
 first1_ergint56 =
   uint8
    5
 ----------
 klasse =
     'uint16'
 min_max =
   1x2 uint16 row vector
        0   65535
 first1_erg0506 =
   uint16
    1
 first1_erg56 =
   uint16
    5
 first1_ergint56 =
   uint16
    5
 ----------
 klasse =
     'int16'
 min_max =
   1x2 int16 row vector
    -32768    32767
 first1_erg0506 =
   int16
    -32767
 first1_erg56 =
   int16
    -32763
 first1_ergint56 =
   int16
    -32763
 ----------
 klasse =
     'single'
 min_max =
      0     1
 first1_erg0506 =
     0.5000
 first1_erg56 =
      1
 first1_ergint56 =
      1
 ----------
 klasse =
     'double'
 min_max =
      0     1
 first1_erg0506 =
     0.5000
 first1_erg56 =
      1
 first1_ergint56 =
      1


Those outputs are different from the results in comment #9.

My implications from this are:

  • giving v=[5 6] does the same, it does not matter if v is double or uint8. I suspect that the CLASS OF V does not matter at all.
  • giving v=[5 6] on an IMAGE I of a INTEGER class, does the first "cut" at the 5th smalles value that is possible in the (integer class) of I. I suspect that for those integer images, the vector v is taken relative to intmin of the image.
  • giving v=[5 6] on an IMAGE I of FLOAT class, does the cut at the value of 1 (probably float inputs are expected to be between 0 and 1, and that's why 5 is kind of Inf in this case). I suspect that for float images, the vector v is cast to a float class (and maybe afterwards trunctated to the interval 0..1)


Does this make sense? Any thoughts?

Hartmut <hardy>
Tue 04 Dec 2018 03:30:56 AM UTC, comment #12: 

Reopening.  Running the example code from hartmut in comment #9, I see that I'm still getting Matlab incompatible results for images of class integer.  I have no clue what is going on.

Carnë Draug <carandraug>
Group Member
Tue 04 Dec 2018 03:02:13 AM UTC, comment #11: 

ok, you see the same as me so I guess it's just not me. It's just Matlab documentation to be very wrong.

I have pushed that fixes this http://hg.code.sf.net/p/octave/image/rev/f45cc1e5eed9

Carnë Draug <carandraug>
Group Member
Mon 03 Dec 2018 09:34:54 PM UTC, comment #10: 

Reopen this bug report, according to previous comments

Avinoam Kalma <avinoam>
Group Member
Mon 03 Dec 2018 01:52:06 PM UTC, comment #9: 

Here is the current Matlab (R2017b I think) behavior:


klassen = {'uint8', 'uint16', 'int16', 'single', 'double'};
for num = 1:length(klassen)
    disp('----------');
    klasse = klassen{num}
    if any(strcmp(klasse, {'uint8', 'int8', 'uint16', 'int16'}))
        vec = [intmin(klasse): intmax(klasse)];
    else
        vec = [0:0.001:1];
        cast(vec, klasse);
    end
    min_max = [min(vec), max(vec)]

    erg05 = grayslice(vec, [0.5]);
    first1_ind = find(erg05);
    first1_erg05 = vec(first1_ind(1))

    erg5 = grayslice(vec, [5]);
    first1_ind = find(erg5);
    first1_erg5 = vec(first1_ind(1))

    int5 = uint8(5);
    ergint5 = grayslice(vec, [int5]);
    first1_ind = find(ergint5);
    first1_ergint5 = vec(first1_ind(1))
end


gives the following results in Matlab:


 ----------
 klasse =
     'uint8'
 min_max =
   1x2 uint8 row vector
      0   255
 first1_erg05 =
   uint8
    1
 first1_erg5 =
   uint8
    51
 first1_ergint5 =
   uint8
    51
 ----------
 klasse =
     'uint16'
 min_max =
   1x2 uint16 row vector
        0   65535
 first1_erg05 =
   uint16
    1
 first1_erg5 =
   uint16
    13107
 first1_ergint5 =
   uint16
    13107
 ----------
 klasse =
     'int16'
 min_max =
   1x2 int16 row vector
    -32768    32767
 first1_erg05 =
   int16
    -32767
 first1_erg5 =
   int16
    -19661
 first1_ergint5 =
   int16
    -19661
 ----------
 klasse =
     'single'
 min_max =
      0     1
 first1_erg05 =
     0.5000
 first1_erg5 =
     0.2000
 first1_ergint5 =
     0.2000
 ----------
 klasse =
     'double'
 min_max =
      0     1
 first1_erg05 =
     0.5000
 first1_erg5 =
     0.2000
 first1_ergint5 =
     0.2000


I have not had a very thorough look at the output. But my conclusions at a first glance would be

  • All results for ergint5 are the same as for erg5. So the class of the V vector does not seem to matter: double(5) gives the same splitting as uint8(5).
  • The vector [0.5] (used for result erg05) never does separate into two halfes. Except for float input images.
  • For integer images, the levels V seem to be understood in the same class


This is very different from what the Matlab help page made me think.

Please have a closer look at those Matlab outputs and let's try to make our function Matlab compatible (and do a better documentation as they did).

Hartmut <hardy>
Sat 01 Dec 2018 01:47:05 PM UTC, comment #8: 

@Hartmut:

The minimal example I showed on comment #6 are the Matlab results (sorry, I should have been more clear).  It shows we are Matlab incompatible for images of non uint8 type (and possibly other integer types).

The thing is that I'm not sure how to get Matlab compatible results. It seems to me that to be Matlab compatible, the second argument (V) needs to be on the same range as the data type of the image. But Matlab documentation specifically documents that values should be in the range [0 1]

From Matlab documentation:

> Note that the threshold values are always between 0 and 1, even if I is of class uint8 or uint16. In this case, each threshold value is multiplied by 255 or 65535 to determine the actual threshold to use.


It's not uncommon for Matlab to be wrong, but this is way too wrong which is why I'm thinking that maybe I'm missing something.

Carnë Draug <carandraug>
Group Member
Fri 30 Nov 2018 10:51:25 AM UTC, comment #7: 

I see this:

  • With image 2.8.0 release and 2.8.1 release:


>> A = grayslice (uint8([0 100 200 255]), [.1 .4 .5])
A = 0  1  3  3
>> B = grayslice (uint8([0 100 200 255]), [100 199 200 210])
B = 0  0  0  4


  • With the current stable branch (only the fixed test from this bug report: the same as above


  • With the current default branch (with Carnes change from comment #6): the same as above


Those results are different from Carnes in comment #6. What version did you use?

I have not had access to Matlab to see what their answer is. But thinking about what the answer should be (following Matlab docs) I would say A= [0 1 3 4] would be good, and B = [0 0 0 0] or [0 0 0 4]. So B might be right, A does NOT look to be correct. Maybe there is only a +-1 wrong somewhere in our code for maxint values?

Hartmut <hardy>
Thu 29 Nov 2018 10:50:27 PM UTC, comment #6: 

I took a look at this tests and found that we are not Matlab compatible. Consider this minimal example:


>> A = grayslice (uint8([0 100 200 255]), [.1 .4 .5])

A =

  1x4 uint8 row vector

   0   3   3   3

>> A = grayslice (uint8([0 100 200 255]), [100 199 200 210])

A =

  1x4 uint8 row vector

   0   1   3   4


The Matlab documentation states that V is always in the [0 1] range and is converted internally for the type of the image but that does not seem to be happening. Can anyone comment?

I get that Matlab documentation is not always right but this is so outrageously wrong that I may be missing something.

In the mean time, I added a bunch of new tests for corner cases.

Carnë Draug <carandraug>
Group Member
Tue 27 Nov 2018 09:54:31 PM UTC, comment #5: 

Pushed to stable
http://hg.code.sf.net/p/octave/image/rev/7ea749493b09
Closing as fixed.

Avinoam Kalma <avinoam>
Group Member
Sat 24 Nov 2018 10:11:20 PM UTC, comment #4: 

Thanks for the review, I will push a revised version

Avinoam Kalma <avinoam>
Group Member
Sat 24 Nov 2018 08:27:31 PM UTC, comment #3: 

I have had a look at this patch (file #45499):

  • You could add some spaces after the function call brackets. For example after "strcmp" or after "class" in the newly added code.


In general, the approach looks fine to me. Since this fix only improves test code and not the "real" function code, I think it could go to the stable branch.

Hartmut <hardy>
Thu 22 Nov 2018 10:28:06 PM UTC, comment #2: 

Since the bug (and the fix) is in the BIST and not in the function itself,
should the fix be done in the stable branch of the image package, or in the dev branch?

Avinoam Kalma <avinoam>
Group Member
Tue 20 Nov 2018 10:13:49 PM UTC, comment #1: 

The problem seems to be with the the test code, and not with
the function.
Please review the attached patch.


(file #45499)

Avinoam Kalma <avinoam>
Group Member
Mon 19 Nov 2018 09:12:35 PM UTC, original submission:  

The following unit test for function grayslice in package image is failing randomly:


>> test grayslice
***** assert (grayslice (im2uint8 (I2d), 3), uint8 (test_grayslice_scalar (I2d, 3)))
!!!!! test failed
ASSERT errors for:  assert (grayslice (im2uint8 (I2d), 3),uint8 (test_grayslice_scalar (I2d, 3)))

  Location  |  Observed  |  Expected  |  Reason
   (2,8)          1            0         Abs err 1 exceeds tol 0 by 1


In the test text


### FIXME investigate why this sometimes fails
%!assert (my_grayslice (im2uint8 (I2d), 3), uint8 (test_my_grayslice_scalar (I2d, 3)))
%!assert (my_grayslice (im2uint16 (I2d), 3), uint8 (test_my_grayslice_scalar (I2d, 3)))


Avinoam Kalma <avinoam>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #50586:  grayslice_int16_V1.patch added by hardy (4KiB - text/x-patch)
file #45499:  grayslice.patch added by avinoam (974B - 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 hardy (Posted a comment)
  • -email is unavailable- added by avinoam (Submitted the item)
  • -email is unavailable- added by avinoam
  • -email is unavailable- added by avinoam
  •  

    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 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-12-29 hardy Attached File- Added grayslice_int16_V1.patch, #50586
    2018-12-04 carandraug StatusFixed In Progress
        Open/ClosedClosed Open
    2018-12-04 carandraug StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2018-12-03 avinoam StatusFixed In Progress
        Open/ClosedClosed Open
    2018-11-27 avinoam StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2018-11-24 avinoam StatusNone Patch Reviewed
    2018-11-20 avinoam Attached File- Added grayslice.patch, #45499
    2018-11-19 avinoam Carbon-Copy- Added hardy
        Carbon-Copy- Added carandraug

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code