I've seen the new file versions (comment #19 and comment #22). There is now even a note about the missing anti-aliasing. Those files look fine to me, I'd be happy to see them pushed to the image repo. Thanks for taking care of this, Avinoam.

As you wish :-)
see attached file.
I have added spaces after function names.

(file #52611)

Now with the files (Thanks @Christof)

(file #52608, file #52609)

I have filed a new bug report (#61760) about antialiasing.

Please review the attached files (the last patch of @Christof with some cosmetic changes).

OK, I filed a new bug report in bug #61754.

I also think anti-aliasing should be a different bug report. It can be fixed there or at least documented as "not implemented". When you open the bug, maybe give a reference to comment #1 in it. Maybe it would also be useful to have the link to the bug report here.

Thanks for the discussion, review and code improvements!

I just had a short look at the more formal aspects of this latest patch (from comment #12). And most things looked fine to me. (Good test coverage, all tests pass, doc strings, coding style.) One single point I noted:

• It seems to me (from the comments in the tests) that Octave's imresize only gives Matlab compatible results, when Matlab runs with the anti-aliasing turned off. If this is true, I think this should be mentioned somewhere in the help string of imresize. (Is this only relevant when shringing images, as suggestd by the Matlab help page?)

I would like Avinoam to also have a last look at the algorithmic logic of this new code. He started to look into this, that's why I hesitate to also dig deeply into this. Afterwards we should hopefully be ready to push this code improvement to the image repository.

One other thing that I realised when reading through the history of this bug report is: a new bug report regarding the incompatibility of the bicubic method in interp2.m (in core Octave) has not been filed so far (as discussed in comment #4 and #5). I would be very good to still do this. Or maybe I have missed this new bug report, then please just mention its bug report number here.

Sorry for my long silence.
I will check the patch, assuming that the patch of bug #60946 will be pushed soon.

Thanks for your comments.

• So, I found a quite recent thread about unit testing local functions. So it is not really possible.

• I seem to have tested everything (partially implicitly) except the padding function. I checked it manually, but I have to add a unit test for it. Actually I can test it indirectly. I haven't thought about it before. So there is no need for making it global.

• I will revert the demo changes. However, I think that RGB images are indeed interpolated channel-wise. Just out of curiosity, what is the alternative from your experience?

I prepared an updated patch (appended), so Avinoam does not need to look at outdated code.

(file #51235)

I have not tried to understand the algorithms and code to do the real calculation here, because I think that Avinoam is already into this topic and will probably continue with this.

Nevertheless I will comment on your discussion points from comment #6:

• testing a local function: I think it is OK to not explicitly test this local function pad_indices in imremap at all. We do still test this local function implicitly, when running the tests in imresize, is this true? I agree, that this would probably not be worth the "mess" of a dedicated subdirectory as with @strel. But if you still consider it useful to test this local function explicitly in some way, you could simply ask on the Octave maintainers email list, probably someone there will have a hint for you how to do this (or confirm your suspicion that it's impossible): -email is unavailable-

• change the demo to color images: I personally do not like this change. Demos are not ment to be tests, they are more kind of educational to show what you can do with a function. And doing interpolation of color images on the 3 color channels seperatly is a bit wrong from the image processing aspect. So I would rather not make this a showcase example, but rather stick with a demo using a grayscale image, here.

• padding option: I think this is fine. Imremap is an Octave only function, so there are no compatibility considerations involved. Addidtionally you only add a feature, you do not remove one, so no long standing user of the image package will be disturbed by this.

• The +-1 difference in output images of integer classes: I think this is fine as well. It is hard to perfectly mimic thte Matlab outputs without actually knowing their code. And your result has 100% the same quality, it is just not 100% compatible to their implementation. A side note: Yes, we are maybe "better" in terms of accuracy with this implementation, but on the other hand we do need more memory for this calculation on integer images, if it is really true that Matlab does the full calculation in the integer domain. Below the bottom line: Our compatibility would still improve a lot to what it currently is.

Just an additional question: Are there now tests included for all the code improvements you did? If you improved the code in aspect A, you should afterwards add a test to check for this aspect A. Otherwise this improvement might get lost after another person, who might not look out for this aspect A, does the next code change to this function.

Thanks for this good code improvment! Let's see if Avinoam has more comments.

OK, I will do that. However, a small benefit from doing it in a function, which is not available in Matlab and hence does not have to be compatible interface-wise, is, that we can have a padding in the interpolation, which is more efficient. Anyway, the most part of the implementation could be moved to interp2, I think.

OK, I made (besides the loop implementation) two vectorized test implementations and compared their performance, see attached script. In terms of Wikipedia:

• The first one uses the formula for W(x).
• The second one uses the formula for p(t).

To my surprise the first one is faster. I will do an implementation in imremap based on the first approach and report back when it's done. Any comments or feedback that I need to consider?


(file #51180)

So, imresize and interp2 in MATLAB use a convolution algorithm, see Wikipedia, with a = -0.5 and a symmetrical padding for the borders. In Octave imresize uses imremap, which uses interp2 and this approximates the derivatives instead of using the convolution algorithm, see Wikipedia. So interp2 is not MATLAB compatible. imremap was implementing an own bicubic interpolation using convolution in 2019, see this commit, but with a = -1 instead of a = -0.5.

Now, there are basically two options:

• Reintroduce the bicubic convolution algorithm in imremap, but with a = -0.5. This would give MATLAB compatibility.
• Stick with interp2 and apply appropriate padding (only for scale > 1). Then we could not test against a MATLAB reference and need a new test.

I don't know what is better. We might want to look at some interpolated images and compare them. The differences are not only near the borders. However, currently there are errors at the border because of a workaround to avoid padding for bilinear interpolation. I made an example to compare the results of both bicubic methods, see attachment bicubic_examples.m. bicubic_conv yields the same results as MATLAB and bicubic_interp uses interp2 with padding.

I read somewhere that MATLAB implements antialiasing by broadening the interpolation convolution kernel. When thinking about implementing antialiasing, it could be useful to have a convolutional implementation of the cubic interpolation. Though there are certainly other ways of implementing antialiasing.

So, what's your opinion?

(file #51178)