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

 
 

bug #45088: [octave forge] (image) rangefilt requires domain and image to have equal number of dimensions

Submitter:  Hartmut <hardy>
Submitted:  Tue 12 May 2015 08:36:39 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  In Progress Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * other Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 06 Nov 2018 09:57:03 PM UTC, comment #25: 

(adding Carne to cc)

Hartmut <hardy>
Tue 06 Nov 2018 09:55:29 PM UTC, comment #24: 

How do we proceed with this?

At the moment (i.e. after pushing the improvements on spatial_filtering.cc with http://hg.code.sf.net/p/octave/image/rev/aecc2d33b0a6 , see bug #54614) the original problem (comment #0) still persists: rangefilt (and several other filtering functions) require domain and image to have the same number of dimensions. This is still needed by (and tested by) the underlying spatial_filtering.cc.

There are several options to proceed with this:

  • (A) We change the m-code in (several) filtering functions, to add padding in each individual filtering function. This is what Avinoam's patches in this bug report started to do, and what Carne does not like (see his comment #19), but what seemd to work fine and in a Matlab compatible way (see comment #21).
  • (B) We do this padding inside of spatial_filtering.cc, this means in c code. This might be not so nice work (at least I am not eager to code this.) This is what Carne proposed (comment #19)
  • (C) We introduce a new m-code function, that does the padding. (Or is there already an existing function that can be used for this?) And we then call this function from inside all the relevant filtering functions.
  • (D) Do don't do anything about this. We only state this behavior correctly in the help text.


My personal preference would be (A). (My second favorite choice would be variant C, but this new m-function will probably only have 5 lines of code and therefore in my opinion not be worth the effort of maintaining a seperate private function.) What do othe people think about this?

Hartmut <hardy>
Wed 23 Aug 2017 08:55:12 AM UTC, comment #23: 

Changing status to "In Progress"

Avinoam Kalma <avinoam>
Group Member
Fri 21 Jul 2017 08:52:07 PM UTC, comment #22: 

There is a new complaint about this on the mailing list (for medfilt2): http://octave.1599824.n4.nabble.com/Error-with-quot-medfilt2-quot-td4684182.html .

Hartmut <hardy>
Wed 29 Mar 2017 06:37:03 PM UTC, comment #21: 

The Matlab result for the rangefilt command in comment #19 is the following:


    19    23    23    15     8
    20    23    23    21    14
    19    19    16    19    19
    14    21    23    23    20
     8    15    23    23    19


There are also a bunch of Matlab compatible tests for rangefilt in file #39544 (from patch #9235).

When I use the patch from comment #16 (file #39535) in rangefilt.m then I get the following result for the rangefilt command in comment #19:


   19   23   23   15    8
   20   23   23   21   14
   19   19   16   19   19
   14   21   23   23   20
    8   15   23   23   19


This is identical to the above Matlab result. So Avinoam's latest patch (file #39535) seems to generate Matlab compatible results even in this new test case. (It also passes all the Matlab compatible tests from the mentioned file #39544.)

Conclusion: No, the (patched) rangefilt.m is not completly wrong. It gives Matlab compatible results in all 14 testcases that we currently have. (The current version in the repo IS completely wrong, though.)

I am very open to deal with the padding issue (in order to better deal with different numbers of dimensions) differently. I might be a clever idea to let spatial_filtering do this padding job. (But I myself wasn't eager to implement this padding in c code.) Now that we have so many test cases available for all filtering functions that call spatial_filtering (see patch #9235) the risk is significantly lower to unintentionally damage something in this code.

Using

imdilate (im, domain) - imerode (im, domain)


also produces the correct result of rangefilt for the new test case in comment #19. (I  haven't tested the results for the other 14 testcases though.) That might be an even better way (faster?) to do a fully new implementation of rangefilt.m. But in this case we still need to fix the calling of spatial_filtering (for entropyfilt and stdfilt) and the padding issue for  the remaining filter functions (probably entropyfilt, stdfilt, medfilt2, ordfilt2, maybe more).



Hartmut <hardy>
Tue 28 Mar 2017 08:56:43 PM UTC, comment #20: 

please see also patch #9235 .

Hartmut <hardy>
Sun 26 Mar 2017 02:45:17 AM UTC, comment #19: 

The error here is the same as what happens in stdfilt, and the same reason why we get an error on medfilt2 and ordfiltn.  And they all call _spatial_filtering_ which is the function that requires input to have the same dimension.  I would prefer to fix _spatial_filtering_ instead to be smarter about it (there's no reason to require the same number of dimensions) than to replicate the padding in all those functions.

But on the specific case of rangefilt, I think that the function is completely wrong. From reading the documentation, I would guess that we could just use:


imdilate (im, domain) - imerode (im, domain)


But the results are different.  Reading the Matlab documentation suggests me the same so I think that rangefilt is just giving completely wrong values:


octave-cli-4.3.0+:16> a = magic(5)
a =

   17   24    1    8   15
   23    5    7   14   16
    4    6   13   20   22
   10   12   19   21    3
   11   18   25    2    9

octave-cli-4.3.0+:17> rangefilt(a, ones(3)) # this looks completely wrong
ans =

   38   34   23   21   18
   37   23   23   25   20
   23   19   22   38   38
   15   22   41   34   32
   19   22   41   34   24


See that the center point of the 5x5 matrix, is not the max - min of the points on that point.

Carnë Draug <carandraug>
Group Member
Tue 24 Jan 2017 09:42:19 PM UTC, comment #18: 

Changing status to "Patch Reviewed". (Thanks Hartmut :-) )

Avinoam Kalma <avinoam>
Group Member
Mon 23 Jan 2017 06:53:24 PM UTC, comment #17: 

I have tested Avinoam's third patch (file #39535):

  • it solves the original issue of this bug report
  • it adds (passing) tests for the mentioned (Matlab compatible) test cases of this bug report
  • it doesn't give any spurious console outputs any more


Thanks for the patch Avinoam. I would be happy to see this one applied to the image repository.

(I will now soon prepare another patch to add more Matlab compatible tests to rangefilt and other related filtering functions. It will appear in the patch tracker, not the bug tracker.)

Hartmut <hardy>
Mon 23 Jan 2017 05:24:56 AM UTC, comment #16: 

Sorry for the delay.
Here is the patch, according to comment #13.

Please review


(file #39535)

Avinoam Kalma <avinoam>
Group Member
Tue 17 Jan 2017 09:31:27 PM UTC, comment #15: 

Ping. Any news here?

(I have generated a bunch of Matlab compatible tests for rangefilt, stdfilt, entropyfilt, medfilt2 and ordfilt2, which I would like to propose as a new patch on the patch tracker, after this rangefilt issue has been fixed.)

Hartmut <hardy>
Mon 02 Jan 2017 04:18:30 PM UTC, comment #14: 

@Hartmut,

Thanks for reviewing and correcting the patch.
I will prepare new cset according to your suggestions

Avinoam Kalma <avinoam>
Group Member
Sun 01 Jan 2017 10:32:26 PM UTC, comment #13: 

Sorry, I think _spatial_filtering_.cc is innocent and works correctly in this case. We are just mis-using it in rangefilt.m.

When I read the help string of ordfiltn.m (to learn about the fourth parameter S in the call to _spatial_filtering_.cc), then I read: "The optional variable S is a matrix of size(DOMAIN). Values of S corresponding to nonzero values of domain are added to values obtained from A when doing the sorting."

So we should probably use only 0s as S in the call to _spatial_filtering_.cc, but currently we are calling it with S = I, the code in ragefilt.m is currently this:


retval = __spatial_filtering__ (I, domain, "range", I, 0);


When I change this line in rangefilt.m in the following way:


retval = __spatial_filtering__ (I, domain, "range", zeros(size(I)), 0);


then I do get the correct result from the code in comment #10.

So I would suggest (after reviewing this) to

  • use Avioams patch from comment #8
  • add the mentioned semicolon to his patch
  • change the fourth parameter in the single code line mentioned in this comment
  • add some new tests, e.g. taken from comment #10 and #0.


@Avinoam: Could you review this and prepare a new cset?

Hartmut <hardy>
Thu 29 Dec 2016 09:11:00 PM UTC, comment #12: 

Yes, the wrong return values seem to be coming from _spatial_filtering_.cc:

I = [ 0   0   1   0   0;
      0   0   1   0   0;
      0   0   0   2   2;
      0   0   0   0   0;
      0   0   0   0   0];
domain = ones(3);
retval = __spatial_filtering__ (I, domain, "range", I, 0)
% -> the center pixel should have value 2, not 3.


Hartmut <hardy>
Thu 29 Dec 2016 08:57:49 PM UTC, comment #11: 
  • What does "K = rangefilt(R)" give in Matlab (with the script from comment #9)?
    • This is ment to answer the question: Are the results of rangefilt.m (with simple 2d input images) currently Matlab compatible at all?


  • I have the suspicion that our current Octave implementation of rangefilt just gives results that are by +1 too big for this input value.
    • The Matlab doc says about rangefilt: "J = rangefilt(I) returns the array J, where each output pixel contains the range value (maximum value − minimum value) of the 3-by-3 neighborhood around the corresponding pixel in the input image I."
    • If the above test gives the expected result "K == J(:,:,1)" then our Octave results are abviously too big by +1 in this case. The difference between the max value and the min value in a 3x3 region of R is 2, and not 3. (See for example the center pixel, min=2, max=4.)
    • This would be a new bug. Presumably in _spatial_filtering_.cc
    • A minimal test code for debugging this new bug is for example this:


X=[0 1 0;0 0 2;0 0 0]
Y=rangefilt(X)
Y(2,2)   % should be 2 (=2-0), not 3



Hartmut <hardy>
Thu 29 Dec 2016 05:56:20 PM UTC, comment #10: 


Matlab output is:


J(:,:,1) =

     1     2     2     2     1
     1     2     2     2     1
     1     2     2     2     1
     1     2     2     2     1
     1     2     2     2     1


J(:,:,2) =

     1     2     2     2     1
     1     2     2     2     1
     1     2     2     2     1
     1     2     2     2     1
     1     2     2     2     1


J(:,:,3) =

     2     4     4     4     2
     2     4     4     4     2
     2     4     4     4     2
     2     4     4     4     2
     2     4     4     4     2


which is different from Octave (after applying my patch).

Changing status to "In Progress"

Avinoam Kalma <avinoam>
Group Member
Wed 28 Dec 2016 11:22:50 PM UTC, comment #9: 

I've tested Avinoam's patch "rangefilt2.diff" from comment #8:

  • There is a semicolon misssing after the new code line with padarray.


  • The patched version of rangefilt.m still supports Nd input images. (see comment #6).


  • The example (with a full white rgb-color input image to rangefilt.m ) from comment #0 works fine with this patch applied. But to judge if this usecase of ndims(I) > ndims(domain) would work fine in general, I would like to know the Matlab result of the following:



R = [1:5] .* ones(5,1);
G = R+1;
B = R*2;
I = cat(3, R, G, B)
J = rangefilt(I)


  • The opposite usecase (mentioned at the end of comment #6) is not dealt with, yet. Carne suggested "If the domain has more dimensions, then the extra dimensions can be removed." But I would like to argue that this usecase is less realistic then my original one, discussed above. The original issue mostly arises because the default value of "domain" is 2d (even with images of higher dimension).



So if the patched version of rangefilt also gives Matlab compatible results on the new test code above, I would suggest to commit this (sligthly corrected) patch to the image repo, close this issue, and do the total rewrite of rangefilt (using imdilate) lateron.

Hartmut <hardy>
Tue 09 Jun 2015 06:22:52 PM UTC, comment #8: 

You are absolutely right (comment #6).
What about something like this? (attached rangefilt2.diff)

(file #34189)

Avinoam Kalma <avinoam>
Group Member
Tue 02 Jun 2015 04:10:06 PM UTC, comment #7: 

Retagging release from 4.0.0-rc4 to 4.0.0.

John W. Eaton <jwe>
Group administrator
Wed 27 May 2015 05:44:35 PM UTC, comment #6: 

The previous code would support N dimensions, the only issue is that both im and filter need to have the same number of dimensions. So no, because the patch would remove features. Also, I'm not that keen on extending functions by only increasing the limit of dimensions by one.

If the image has more dimensions, the domain can be padded with zeros. If the domain has more dimensions, then the extra dimensions can be removed.

Carnë Draug <carandraug>
Group Member
Tue 26 May 2015 03:36:39 PM UTC, comment #5: 


What do you think about the following diff file?
It supports 2 and 3 dimensional images (and NOT n-dimensional like
Matlab).
The diff is wrt default branch (and not the stable).




(file #34100)

Avinoam Kalma <avinoam>
Group Member
Sun 24 May 2015 04:23:28 PM UTC, comment #4: 

There is no confusion. It simply assumes that they have the same number of dimensions. That makes sense since _spatial_filtering_ itself will not be able to handle them otherwise. So the required fix is adding support for such cases.

Carnë Draug <carandraug>
Group Member
Wed 13 May 2015 10:19:06 AM UTC, comment #3: 


Oops, problem with the verbatim line.
(there should be an option to view the comment before submitting it)

The error message is from the line


    idx {k} = (even (k)+1):size (I, k);


even has size of 2, but k goes from 1 to 3.
It seeme that there is a confusion between the dimensions of
the domain matrix, and the dimensions of the image matrix.

The same problem (and the same solution?) in stdfilt.m

Avinoam Kalma <avinoam>
Group Member
Wed 13 May 2015 10:15:10 AM UTC, comment #2: 


The error message is from the line


   idx {k} = (even (k)+1):size (I, k);
-verbatim+

even has size of 2, but k goes from 1 to 3.
It seeme that there is a confusion between the dimensions of
the domain matrix, and the dimensions of the image matrix.

The same problem (and the same solution?) in stdfilt.m

Avinoam Kalma <avinoam>
Group Member
Wed 13 May 2015 12:30:35 AM UTC, comment #1: 

I confirm this problem. The problem has to do how the padding is done.  The function that does all the work, _spatial_filtering_, requires things to have the same number of dimensions. A workaround for this is:


pkg load image
I = ones(10,10,3);
J = rangefilt (I, padarray (ones (3), [0 0 1]));


This could be fixed by padding the things properly inside rangefilt (see psf2otf for some tips on how to handle padding of things and arbitrary number of dimensions).  However, it seems to me that this whole function could be rewritten to use imerode and imdilate for some performance as well.

Carnë Draug <carandraug>
Group Member
Tue 12 May 2015 08:36:39 PM UTC, original submission:  

Here is a little script to show the bevahior (image-2.4.0 on Octave 4.0.0-RC4):


pkg load image
I = ones(10,10,3);
J = rangefilt(I);


Even after fixing bug #45087 this still does NOT work, and produces an error message. It seems to me that the Octave implementation of rangefilt.m does not currentely support rgb images.

This limitation is not mentioned in the help text of rangefilt.m and I think Matlab works fine with rgb images.

Can someone improve rangefilt, or should we just add this limitation info to its help text?

Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #39535:  rangefilt_v3.cs added by avinoam (1KiB - text/plain)
file #34189:  rangefilt2.diff added by avinoam (544B - application/octet-stream)
file #34100:  rangefilt.diff 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 mmuetzel (Updated the item)
  • -email is unavailable- added by hardy
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by hardy (Submitted the item)
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2020-12-02 mmuetzel Release4.0.0 other
    2018-11-06 hardy Carbon-Copy- Added carandraug
    2017-08-23 avinoam StatusPatch Reviewed In Progress
    2017-08-13 jwe Summaryimage package: rangefilt requires domain and image to have equal number of dimensions [octave forge] (image) rangefilt requires domain and image to have equal number of dimensions
    2017-01-24 avinoam StatusPatch Submitted Patch Reviewed
    2017-01-23 avinoam StatusIn Progress Patch Submitted
    2017-01-23 avinoam Attached File- Added rangefilt_v3.cs, #39535
    2017-01-02 avinoam Summaryimage package: rangefilt requires domain and image to have equa number of dimensions image package: rangefilt requires domain and image to have equal number of dimensions
    2016-12-29 avinoam StatusPatch Submitted In Progress
    2016-12-12 avinoam StatusNone Patch Submitted
    2015-06-09 avinoam Attached File- Added rangefilt2.diff, #34189
    2015-06-02 jwe Release4.0.0-rc4 4.0.0
    2015-05-26 avinoam Attached File- Added rangefilt.diff, #34100
    2015-05-13 carandraug Summaryimage package: rangefilt doesn\'t work on RGB images image package: rangefilt requires domain and image to have equa number of dimensions

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code