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

 
 

bug #54614: [octave forge] (image) wrong results of entropyfilt

Submitter:  Avinoam Kalma <avinoam>
Submitted:  Mon 03 Sep 2018 05:16:00 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Avinoam Open/Closed:  * Closed
Release:  * other Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 10 Oct 2018 01:09:29 PM UTC, comment #9: 


> The function spatial_filtering should also have a check that S and DOMAIN have the exact same dimensions.


Now done with http://hg.code.sf.net/p/octave/image/rev/aecc2d33b0a6

I have pushed the rest of the patch with http://hg.code.sf.net/p/octave/image/rev/ae48f7ddee82 which fixes the issues and so I'm closing it.

Hartmut wrote on comment #7:

> Do you have a clever idea, how to:
> * use spatial_filtering (keeping its 4t input argument alive)
> * not use more memory than necessary
> * and still be able to tell spatial_filtering in most use cases to NOT add this forth argument to the result?
> Maybe we could find a trick inside spatial_filtering to treat an EMPTY forth argument as a matrix of zeros (of size(I)) , but without actually requirement memory for all those zeros? Do you have a clever idea how to implement this in spatial_filtering?


The memory usage is not a problem because the 4th input argument is an array the same size as the domain which is typically much smaller than the actual image. The issue is the extra computations since we will be adding zero many times. I think this could be avoided with templates which should also make the code simpler to follow. Not sure it's worth the time, but feel free to give it a go.

Carnë Draug <carandraug>
Group Member
Wed 10 Oct 2018 10:25:08 AM UTC, comment #8: 


> Is there any (current) use (in the Octave image toolbox code base) of spacial_filtering's capability to add an additional image argument (argument number 4)?


At least the functions ordfilt makes use of it correctly and exposes it as a user option.

> Fixing stdfilt is also done in patch #9235, it just puts 0s instead of a second copy of the image I, into the 4th input argument of spacial_filtering.


The function spatial_filtering should also have a check that S and DOMAIN have the exact same dimensions.

Carnë Draug <carandraug>
Group Member
Tue 09 Oct 2018 08:03:08 PM UTC, comment #7: 

The general scheme in patch #9235 is the following (for example in entropyfilt.m):

-  retval = __spatial_filtering__ (I, domain, "entropy", I, nbins);
+  retval = __spatial_filtering__ (I, domain, "entropy", zeros (size (I)), nbins);


The problem with this approach is (I think Carne mentioned this a long while ago), that we then use double the amount of memory. And the memory for the zeros is not very useful at all :(

Do you have a clever idea, how to

  • use _spatial_filtering_ (keeping its 4t input argument alive)
  • not use more memory than necessary
  • and still be able to tell _spatial_filtering_ in most use cases to NOT add this forth argument to the result?


Maybe we could find a trick inside _spatial_filtering_ to treat an EMPTY forth argument as a matrix of zeros (of size(I)) , but without actually requirement memory for all those zeros? Do you have a clever idea how to implement this in _spatial_filtering_?

Hartmut <hardy>
Tue 09 Oct 2018 07:28:21 PM UTC, comment #6: 

Thanks, now I understand comment #1.
We should apply the patch #9235, to fix the calling to
__spacial_filtering__, and then fix the normalization of the histogram

In this stage I prefer not to change __spacial_filtering__ (remove 4th argument).

I will review patch #9235

Avinoam Kalma <avinoam>
Group Member
Mon 08 Oct 2018 06:36:30 PM UTC, comment #5: 

Fixing stdfilt is also done in patch #9235, it just puts 0s instead of a second copy of the image I, into the 4th input argument of _spacial_filtering_. Both ways (comment #4 and patch #9235) effectivly remove the second summand in _spacial_filtering_.
 This fix should also (in either of those ways) be applied to several other filtering functions (as listed in patch #9235).

How should be proceed with this? The question is: Is there any (current) use (in the Octave image toolbox code base) of _spacial_filtering_'s capability to add an additional image argument (argument number 4)? This capability is documented in spacial_filtering I think. But this helper function is "Octave only" and so we are not constrained by Matlab compatibility in this case. We could decide to simply remove this capability from _spacial_filtering_.cc. What do you think?

Hartmut <hardy>
Sun 07 Oct 2018 08:58:36 PM UTC, comment #4: 

MATLAB:

>> stdfilt (magic(5))

 
ans =

    6.0438    9.7568    8.7433    5.8973    3.2059
    8.8318    8.5065    7.4907    6.6039    4.4190
    7.4963    6.5192    6.1237    6.5192    7.4963
    4.4190    6.6039    7.4907    8.5065    8.8318
    3.2059    5.8973    8.7433    9.7568    6.0438

Octave:

>> stdfilt (magic(5))


ans =

   12.0876   10.4854    9.0108    8.6168    6.9901
   10.5567    7.7460    7.8440    8.5310    7.0534
    8.2614    7.1783    8.4574   12.0012   11.5302
    5.7542    7.6649   13.0384   10.6784   10.2280
    5.8949    7.6721   12.5100   11.0227    7.5829


After changing
 

         values_fvec[se_ind] = in(in_sub) + heights_fvec[se_ind];


to


         values_fvec[se_ind] = in(in_sub); // + heights_fvec[se_ind];


 

>> stdfilt (magic(5))


ans =

    6.0438   9.7568   8.7433   5.8973   3.2059
   8.8318   8.5065   7.4907   6.6039   4.4190
   7.4963   6.5192   6.1237   6.5192   7.4963
   4.4190   6.6039   7.4907   8.5065   8.8318
   3.2059   5.8973   8.7433   9.7568   6.0438


Avinoam Kalma <avinoam>
Group Member
Sat 29 Sep 2018 01:23:54 PM UTC, comment #3: 


> Another problem is in the transfer of data from the matrix:

It seems that the line [...] is wrong [...]

That line adds the height value to each element. Some functions call _spatial_filtering_ with a value there (but I think in most cases it will be zero).

Do you have a case where that line is causing incorrect values? If the test suite did not fail when you removed it, I guess we should add a case for it. For example:


octave:7> ordfiltn([1 2 3 4], 1, [1 1 1], [0 0 0]) # this is default height (S)
ans =

   0   1   2   0

octave:8> ordfiltn([1 2 3 4], 1, [1 1 1], [2 1 0])
ans =

   2   3   4   0


Carnë Draug <carandraug>
Group Member
Sat 08 Sep 2018 09:03:17 PM UTC, comment #2: 

Thanks for pointing me related bugs, however, I found some other problems:

Checking entropyfilt, there are two problems in __spatial_filtering__.cc

The normalization of the histogram is wrong. Instead of


  for (octave_idx_type i = 0; i < len; i++)
    hist (vals (i) + add) /= (double)len;


the loop should be


  for (octave_idx_type i = 0; i < nbins; i++)
    hist (i) /= (double)len;


Another problem is in the transfer of data from the matrix:
It seems that the line


         values_fvec[se_ind] = in(in_sub) + heights_fvec[se_ind];


is wrong, and should be replaced by


         values_fvec[se_ind] = in(in_sub) ;


(Can someone confirm this?)
This change affects also stdfilt, rangefilt, and ordfiltn.

Changeset attached, including relevant tests in entropyfilt.m

Please review.

(file #44959)

Avinoam Kalma <avinoam>
Group Member
Sat 08 Sep 2018 06:21:03 PM UTC, comment #1: 

Please also have a look at file #39544 in my patch #9235 (and maybe bug #45088 as well). It deals with a bunch of filter functions at once, including entropyfilt.m. This patch isn't perfect (the results aren't Matlab compatible, yet), but it

  • fixes the call to spatial_filtering.cc
  • has a bunch of Matlab compatible tests for entropyfilt in it.


Some years ago, Carne didn't want to use this patch, because he preferred to rather change the code of spatial_filtering.cc directly (if I remember correctly). But neither of us has taken the time to rewrite my patch since then...

Note: I haven't checked if this patch (i.e. only correcting the call to spatial_filtering.cc inside of entropyfilt.m) improves the results on the sample date in comment #0.

Hartmut <hardy>
Mon 03 Sep 2018 05:16:00 PM UTC, original submission:  


Example:

Matlab:


>> entropyfilt (uint8(magic(5)),ones(3))

ans =

    1.8366    2.5033    2.5033    2.5033    1.8366
    2.5033    3.1699    3.1699    3.1699    2.5033
    2.5033    3.1699    3.1699    3.1699    2.5033
    2.5033    3.1699    3.1699    3.1699    2.5033
    1.8366    2.5033    2.5033    2.5033    1.8366


Octave:

>> entropyfilt (uint8(magic(5)), ones(3))
ans =

   0.62242   1.45219   1.45219   1.45219   1.00074
   1.45219   3.16993   3.16993   3.16993   3.16993
   1.45219   3.16993   3.16993   3.16993   3.16993
   1.45219   3.16993   3.16993   3.16993   3.16993
   1.45219   3.16993   3.16993   3.16993   3.16993


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 #44959:  bug_54614_V1.cs added by avinoam (2KiB - 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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-10 carandraug StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2018-09-13 avinoam StatusNone Patch Submitted
    2018-09-08 avinoam Attached File- Added bug_54614_V1.cs, #44959
    2018-09-03 avinoam Carbon-Copy- Added carandraug
        Carbon-Copy- Added hardy

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code