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

 
 

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

Submitted by:  Avinoam Kalma <avinoam>
Submitted on:  Mon 19 Nov 2018 09:12:35 PM UTC  
 
Category:  Octave Forge 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

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

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):

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:

gives the following Matlab output:

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

gives the following Matlab result:

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:

And here are the corresponding Matlab results:

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>
Project 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>
Project Member
Mon 03 Dec 2018 09:34:54 PM UTC, comment #10:

Reopen this bug report, according to previous comments

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

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

gives the following results in Matlab:

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>
Project 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:
  • 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:

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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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:

In the test text

Avinoam Kalma <avinoam>
Project Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
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
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    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.3