Changing status to "In Progress"

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

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

please see also patch #9235 .

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.

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

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

Sorry for the delay.
Here is the patch, according to comment #13.
Please review
(file #39535)

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,
Thanks for reviewing and correcting the patch.
I will prepare new cset according to your suggestions

Sorry, I think _spatial_filtering_.cc is innocent and works correctly in this case. We are just misusing 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?

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

 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 3by3 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:

Matlab output is:
which is different from Octave (after applying my patch).
Changing status to "In Progress"

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

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

Retagging release from 4.0.0rc4 to 4.0.0.

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.

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

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.

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

The error message is from the line

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.

Here is a little script to show the bevahior (image2.4.0 on Octave 4.0.0RC4):
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?
