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

 
 

bug #54974: [octave forge] (image) incompatible results of entropy.m

Submitted by:  Hartmut <hardy>
Submitted on:  Tue 06 Nov 2018 08:05:58 PM UTC  
 
Category:  Octave Forge 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

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)

Tue 20 Nov 2018 06:33:09 PM UTC, comment #14:

Closing as fixed.

Avinoam Kalma <avinoam>
Project Member
Sun 18 Nov 2018 08:05:00 PM UTC, comment #13:

Looks good.
pushed http://hg.code.sf.net/p/octave/image/rev/c90b489def49
If there are no further comments, I will close this bug report.
Thanks :-)

Avinoam Kalma <avinoam>
Project Member
Thu 15 Nov 2018 06:41:20 AM UTC, comment #12:

I've enlarged the information about this change in the NEWS file, because the output values of this function are now always different than before. See attached patch (V5).

(file #45428)

Hartmut <hardy>
Wed 14 Nov 2018 09:41:31 PM UTC, comment #11:

Here is a new proposal for a patch (file V4):

  • I tried to adjust all issues from Carne's comment #9. (Sorry if I missed a whitespace somewhere...)
  • I changed the input checking of I, to allow for logical input images.
  • I added some tests for logical input images.

Please review this and let me know if something is still wrong before pushing.

@Avinoam: After finishing this, I will proceed with patch #9235, because I would like to use this corrected entropy function in the BISTs there.

(file #45425)

Hartmut <hardy>
Wed 14 Nov 2018 08:53:39 AM UTC, comment #10:

Hartmut - could please update also patch #9235?

Avinoam Kalma <avinoam>
Project Member
Tue 13 Nov 2018 03:25:42 PM UTC, comment #9:

Looks fine to me, if it fixes the issue. Just:

1) the patch adds trailing whitespace. Please remove
2) can you add a test case for logical input as well
3) list entropy in the NEWS file (if we keep the NEWS file up to date, then it's easier to make the release - alternative to check history when making the release)

Carnë Draug <carandraug>
Project Member
Mon 12 Nov 2018 08:16:45 PM UTC, comment #8:

Here is a new version of my patch (V3) with additional (Matlab compatible) tests using uint8 inputs.

Please review if this one is now ready for pushing.

(file #45416)

Hartmut <hardy>
Fri 09 Nov 2018 01:12:28 PM UTC, comment #7:

Could you add some 8 bit tests like

assert (entropy (uint8(C)), ...., eps)

Avinoam Kalma <avinoam>
Project Member
Thu 08 Nov 2018 10:11:11 PM UTC, comment #6:

This was a good idea to imcast float images to uint8, it made our function Matlab compatible for all the float images I tested :)

Here is a new patch file (V2). It:

  • uses im2uint8 on all input images except for logicals (as Matlab claims to do)
  • uses imhist instead of hist, because Matlab does it (according to its help page)
  • it removes the zero elements in the histogram p properly. (I double checked the previous code line "P += (P == 0);". It puts a 1 at all 0 positions, which excludes those histogram bins from the entropy calculation because log2(1)=0. But the resulting histogram p cannot easily be normalized to a sum of 1 afterwards, because there are all those surplus 1s in it.)
  • it normalizes the histogram p to a sum of 1 (this is how I understand the word "normalized histogram counts" in the Matlab help page. And it works fine.)
  • it adds a bunch of Matlab compatible tests with float images.

Any comments on this new patch? Do you any objections to push this patch?

(file #45388)

Hartmut <hardy>
Tue 06 Nov 2018 09:21:22 PM UTC, comment #5:

Yes, it seems so.
Maybe imcast to uint8 can help?

Avinoam Kalma <avinoam>
Project Member
Tue 06 Nov 2018 09:13:56 PM UTC, comment #4:

Is it really true that the current Octave implementation is (quite) compatible for uint8 intputs, but nor for float inputs? In this case I would also like to conclude that we still do something wrong in the first "conversion to uint8" step.

Hartmut <hardy>
Tue 06 Nov 2018 08:25:50 PM UTC, comment #3:

Reading Matlab documentation:

"entropyfilt converts any class other than logical to uint8 for the histogram count calculation so that the pixel values are discrete and directly correspond to a bin value",

So if you use in the examples:

You will see that Octave results are similar to Matlab results, on uint8 matrices.

Avinoam Kalma <avinoam>
Project Member
Tue 06 Nov 2018 08:19:57 PM UTC, comment #2:

(properly adding Carne to cc)

Hartmut <hardy>
Tue 06 Nov 2018 08:17:24 PM UTC, comment #1:

Here is my first proposal how to fix this (see patch file V1):

  • Use imhist instead of hist.

**This is what the Matlab help on entropy claims to do. I'm not sure this is necessary.

  • Normalize the resulting histogramm to have a sum of 1.
    • This normalization is also mentioned on the Matlab help page. And when I use this my results becomse compatible.
  • Ignore the zero elements in the histogramm.
    • This was also done before (as the code comment sais), but I do not understand what entropy.m did before, the code was P += (P == 0);
  • I added the example from comment #0 as new test case.

Please have a look and review this test. Any comments on the above mentioned changes are appreciated.

I would espacially appreciate of someone could check of the new test case is Matlab compatible (as I am currently only assuming this). And one or two more Matlab compatible test cases with real numbers (e.g. 5 to 10 element matrices) could also be useful.

(file #45372)

Hartmut <hardy>
Tue 06 Nov 2018 08:05:58 PM UTC, original submission:

The results of entropy.m in the image package don't seem to be very Matlab compatible.

Here is a small example:

From another bug report (on entropyfilt, see patch #9235) I assume that the Matlab result on this will be 0.9183. But the current (image 2.8.0 release) Octave output is 2.9477.

Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

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

    Date Changed by Updated Field Previous Value => Replaced by
    2018-11-20 avinoam StatusPatch Submitted => Fixed
        Open/ClosedOpen => Closed
    2018-11-15 hardy Attached File- => Added entropy_compatibility_V5.patch, #45428
    2018-11-14 hardy Attached File- => Added entropy_compatibility_V4.patch, #45425
    2018-11-12 hardy Attached File- => Added entropy_compatibility_V3.patch, #45416
    2018-11-09 avinoam StatusConfirmed => Patch Submitted
    2018-11-08 hardy Attached File- => Added entropy_compatibility_V2.patch, #45388
    2018-11-06 avinoam Release4.4.1 => other
    2018-11-06 avinoam StatusNone => Confirmed
    2018-11-06 hardy Carbon-Copy- => Added carandraug
    2018-11-06 hardy Carbon-CopyRemoved randraug => -
    2018-11-06 hardy Attached File- => Added entropy_compatibility_V1.patch, #45372
    2018-11-06 hardy Carbon-Copy- => Added randraug
        Carbon-Copy- => Added avinoam

    Back to the top


    Powered by Savane 3.3