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

 
 

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

Submitter:  Hartmut <hardy>
Submitted:  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
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( 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]
entropy(G)

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
  •  

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