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

Submitted by:  Hartmut <hardy>
Submitted on:  Tue 12 May 2015 08:36:39 PM UTC  
 
Category: Octave Forge PackageSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: In ProgressAssigned to: None
Originator Name: Open/Closed: Open
Release: 4.0.0Operating 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 Jump to the original submission)

Wed 23 Aug 2017 08:55:12 AM UTC, comment #23:

Changing status to "In Progress"

Avinoam Kalma <avinoam>
Project 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:

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:

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

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:

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

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

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

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

Avinoam Kalma <avinoam>
Project 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>
Project 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>
Project 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:

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

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:

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:
Hartmut <hardy>
Thu 29 Dec 2016 05:56:20 PM UTC, comment #10:

Matlab output is:

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

Changing status to "In Progress"

Avinoam Kalma <avinoam>
Project 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:
  • 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>
Project 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>
Project 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>
Project 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>
Project 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>
Project 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

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>
Project Member
Wed 13 May 2015 10:15:10 AM UTC, comment #2:

The error message is from the line

Avinoam Kalma <avinoam>
Project 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:

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>
Project 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):

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 File(s):
   
   
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
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by avinoam (Posted a comment)
  • -unavailable- added by hardy (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 12 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 23 Aug 2017 08:55:12 AM UTCavinoamStatusPatch Reviewed=>In Progress
    Sun 13 Aug 2017 12:49:28 PM UTCjweSummaryimage 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
    Tue 24 Jan 2017 09:42:19 PM UTCavinoamStatusPatch Submitted=>Patch Reviewed
    Mon 23 Jan 2017 05:27:55 AM UTCavinoamStatusIn Progress=>Patch Submitted
    Mon 23 Jan 2017 05:24:56 AM UTCavinoamAttached File-=>Added rangefilt_v3.cs, #39535
    Mon 02 Jan 2017 04:18:30 PM UTCavinoamSummaryimage 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
    Thu 29 Dec 2016 05:56:20 PM UTCavinoamStatusPatch Submitted=>In Progress
    Mon 12 Dec 2016 05:54:23 AM UTCavinoamStatusNone=>Patch Submitted
    Tue 09 Jun 2015 06:22:52 PM UTCavinoamAttached File-=>Added rangefilt2.diff, #34189
    Tue 02 Jun 2015 04:10:06 PM UTCjweRelease4.0.0-rc4=>4.0.0
    Tue 26 May 2015 03:36:39 PM UTCavinoamAttached File-=>Added rangefilt.diff, #34100
    Wed 13 May 2015 12:30:35 AM UTCcarandraugSummaryimage 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.1-cleanup1