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:

C = uint8([1 1 1; 2 2 2; 3 3 3]);
D = C';
E = uint8(ones (3,3));
E(2,2) = 2;
F = uint8(3 .* ones (3,3));

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:

G = [-1 2 7; -5 2 8; -7 pi 9]

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:

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.5