patchGNU Octave - Patches: patch #9354, image package: new function...

 
 

patch #9354: image package: new function wiener2.m

Submitter:  Hartmut <hardy>
Submitted:  Tue 23 May 2017 09:18:18 PM UTC
   
 
Category:  Forge : new function Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 20 Dec 2018 04:22:49 AM UTC, comment #20: 

I went to pick up the old commits that added ND support, as well as your new tests and pushed this. Closing.

Carnë Draug <carandraug>
Group Member
Sun 02 Dec 2018 09:34:43 PM UTC, comment #19: 

@Carne: Should we finalize this? I think what is still missing is:


Or are there more issues still open (maybe from your comment #10 ) ?

Hartmut <hardy>
Wed 21 Jun 2017 05:40:33 PM UTC, comment #18: 

The idea with making our new wiener2.m deal with Nd images was good. Pyton's SciPy is doing it as well, they call it scipy.signal.wiener :



Hartmut <hardy>
Mon 12 Jun 2017 06:08:50 PM UTC, comment #17: 

Here are 3 new tests for neighborhoods with even size:


im1 = zeros (5, 5);
im1(2:4, 2:4) = 1;

im1_out_23 = [
    0.1667    0.2233    0.2978    0.2233    0.1667
    0.2233    0.7767    1.0000    0.7767    0.2233
    0.2233    0.7767    1.0000    0.7767    0.2233
    0.1667    0.5533    0.7022    0.5533    0.1667
         0         0         0         0         0];

im1_out_43 = [
    0.1667    0.2813    0.3750    0.2813    0.1667
    0.2500    0.6250    0.7500    0.6250    0.2500
    0.2500    0.6250    0.7500    0.6250    0.2500
    0.1667    0.4375    0.6250    0.4375    0.1667
    0.0833    0.1667    0.2500    0.1667    0.0833];

im1_out_44 = [
    0.2500    0.3018    0.3018    0.2500    0.1250
    0.3018    0.6647    0.6647    0.4971    0.1875
    0.3018    0.6647    0.6647    0.4971    0.1875
    0.2500    0.4971    0.4971    0.2500    0.1250
    0.1250    0.1875    0.1875    0.1250    0.0625];

assert (wiener2 (im1, [2, 3]), im1_out_23, 1e-4);
assert (wiener2 (im1, [4, 3]), im1_out_43, 1e-4);
assert (wiener2 (im1, [4, 4]), im1_out_44, 1e-4);


You could simply add those tests under the already existing test with "im1".

Those tests

  • are Matlab compatible
  • and pass


So everything seems fine for even sized neighborhoods.

Hartmut <hardy>
Sun 11 Jun 2017 06:51:05 PM UTC, comment #16: 

Here is the underlying reason why the new tests for Nd images failed for me (see comment #11 and comment #14): I used the fspecial.m file from the latest (2.6.1) image release. This still canNOT deal with 3d neighborhoods:


>> pkg list
Package Name  | Version | Installation directory
--------------+---------+-----------------------
       image *|   2.6.1 | /home/hartmut/octave/image-2.6.1

>> fspecial ("average", [3 3 3])
ans =

   0.11111   0.11111   0.11111
   0.11111   0.11111   0.11111
   0.11111   0.11111   0.11111


When I switch to the latest version of fspecial.m (in the official image repo) then all the test of wiener2.m now do PASS on my machine as well. No need to change those new tests. So I was only misled by the commit message on the change of fspecial.m. This change was not only about performance, it also make Nd neighborhoods for the average filter possible.

Conclusion: Are the maybe many more places in fspecial.m, that currently do not support the creation of Nd filter matrices (and silently return a 2d matrix)?

(I'll probably come up with new tests for wiener2 with even sized neighborhoods, tomorrow.)

Hartmut <hardy>
Sun 04 Jun 2017 06:39:18 PM UTC, comment #15: 

I'll have a look at this after my vacation...

Hartmut <hardy>
Sun 04 Jun 2017 11:23:39 AM UTC, comment #14: 

Thank you for the tests and comments.  I added your new tests, fixed
the docs, adjusted input check, and made imcast support class logical.

https://bitbucket.org/carandraug/octave-image/commits/8a7e8063889672a98fed99d15b73d46a1c8a245c

Could you find write some tests for what happens with even sized nhood
such as [2 3], [4 3], and [4 4]?

> Your new test fails on my linux machine. The absolute errors are on
> the order of 1e-4 (except for the 5,5,5 position in one result
> image, this is off by 0.0018.) Maybe you should add a looser
> tolerance to this new test, or is there a typo somewhere?


There is no typo.  This works on my machine.  I have seen some tests
failing because of machine precision but that was rarely for more than
1-2 orders of magnitude.  You are suggesting changing from eps to 1e-4
which is a very large difference.  I'd be curious to find out why the
big difference.

Carnë Draug <carandraug>
Group Member
Fri 02 Jun 2017 11:55:47 AM UTC, comment #13: 

Here is the Matlab result for more exotic input types:

  • wiener2([])  ->   []


  • wiener2(logical([0 0; 1 1]))  -> logical(ones(2,2))


This is NOT what our current Octave implementation of wiener2.m does. (Because this is NOT what the Matlab help says: "Class Support: The input image I is a two-dimensional image of class uint8, uint16, int16, single, or double. The output image J is of the same size and class as I.")

So we should probably change our input checks accordingly, and add the above two lines as new tests to the function.


Hartmut <hardy>
Thu 01 Jun 2017 12:05:31 PM UTC, comment #12: 

Here is a Matlab result for a 1d image:

wiener2(ones(1,5))
ans = 0.2222    0.3926    0.3926    0.3926    0.2222


This is the same result as with the current (Carne repo) version of Octave's wiener2. So, yes, they are also using a [3,3] neighborhood for 1d images.

Could you add this 1d result as a test to your wiener2 file, Carne?

The results for empty and logical inputs will come later.

Additonal note: There is still a bug in the help string of the current repo version, Carne. It now looks like this:

 -- Function File: J =  wiener2 (I)
 -- Function File: J =  wiener2 (I, NHOOD])
 -- Function File: J =  wiener2 (I, NOISE)
 -- Function File: J =  wiener2 (I, NHOOD], NOISE)
 -- Function File: [J, NOISE] = wiener2 (I, [M, N])
Apply an adaptive noise reduction filter.

I think the two lonely closing square brackets should go. And you probably want to change the last J=... line to nhood as well.

Hartmut <hardy>
Wed 31 May 2017 08:53:34 PM UTC, comment #11: 

Thanks for adding the Nd support to the wiener2 function.

I've had a look at your mentioned changeset in your bitbucket repository:

  • I think your default value for an Nd neighborhood is generally reasonable. Also for singleton dimensions. But the most commmon usecase where this will not be what the user expects are RGB images. For RGB images my expected default value, if just using wiener2(im), would be nhood=[3 3 1]. But I can well imagine other 3d data where nhood=[3 3 3] is a more natural default neighborhood. Maybe we should just write another sentence to the help string to explain how to properly use this function on RGB images?


  • I haven't thorougly check your input checks and parameter juggling. But it does look reasonable. And my old tests still pass.


  • Your new test fails on my linux machine. The absolute errors are on the order of 1e-4 (except for the 5,5,5 position in one result image, this is off by 0.0018.) Maybe you should add a looser tolerance to this new test, or is there a typo somewhere?


I will check the Matlab behavior on logical and empty inputs later. I will also check the Matlab result for wiener2(ones(1,5)), to make sure how 1d inputs are treated, I had not checked this before (onyl having 2d images in mind).

Hartmut <hardy>
Wed 31 May 2017 12:21:51 AM UTC, comment #10: 

I pushed this http://hg.code.sf.net/p/octave/image/rev/7e54312e1df2 which adds your wiener2 function to the image package.

I expanded it to work for n dimensions with this https://bitbucket.org/carandraug/octave-image/commits/ac1f6b2e230e0f5f860d7c8a9ee2435497b124ed  could you take a look? Even without the reason to support nd images, this uses convn directly which will be faster.  And turns out there's no need to use padarray because convn already does the padding with zeros.

I don't think we need to check for ndims(image)==numel(neighborhood) or to postpad nhood, since convn can handle mixed number of dimensions on both arrays.  I was unsure about how to handle singleton dimensions and its effect on the default nhood.  Should a [10 10 1 4] array default to a [3 3 3 3] or a [3 3 1 3] nhood?  I went with [3 3 3 3] because that's consistent with what happens at the moment for 1d arrays, i.e., an image of size [10 1] still uses a [3 3] nhood.  Or is this wrong?  Similarly, if a user specifies a nhood with more dimensions than the image, it's their business, we shouldn't give an error.

Also, can you confirm on recent Matlab that IM indeed cannot be logical or empty for wiener2 (similar to the issue on mean2?)

Carnë Draug <carandraug>
Group Member
Tue 30 May 2017 06:15:22 PM UTC, comment #9: 

I've submitted bug #51144 and bug #51144 for the root causes of the behavior mentioned in the fourth point of comment #2 and comment #3. (I.e. funny inputs to the function.)

Hartmut <hardy>
Sat 27 May 2017 10:03:31 AM UTC, comment #8: 

I've done the proposed changes:

https://bitbucket.org/hgimpel/octave-image/commits/e58325fdbe19ae3f4db066302d64542023937d2f

Regarding to question 3: In the literature I have only seen this method applied to 1d or 2d images. But since it only relies on the noise variance (implicitly assuming additive gaussian noise), the image variance and the image mean in some simple neighborhood, it should also work fine with Nd images. The following changes would be required for this:

  • skip the input test of ndims==2 for image and neighborhood
  • add an input test of ndims(image)==numel(neighborhood), maybe even postpad neighborhood with 1s beforehand.
  • make sure the two filtering commands works fine (currently imfilter only supports 2d images, right? Maybe switch to convn and padarray as suggested for the meantime.)
  • mention it in the documentation
  • add a test for it

I don't know if Matlab allows for Nd input images. But this shouldn't keep us from doing better than them.

I will try to figure out how to create new named bookmarks in tortoise-hg. Maybe next time I can use them instead of branches.

Hartmut <hardy>
Sat 27 May 2017 01:06:11 AM UTC, comment #7: 

Four comments:

1. instead of checking if it's of class double, I think it's better to just use isfloat.  That will prevent the conversion to double if input is of class single but that is the whole point of it. If someone uses single instead of double they already made the choice of paying precision to save memory.

2. just use 'mean (x(:))' instead of 'mean2 (x)' which is clearer, and returns the same. mean2 is really only for sake of matlab compatibility.

3. if the use of imfilter was replaced with padarray and convn, is there anything preventing it from working with nd dimensions? (despite the name wiener2)

4. at the end you could return mean_im and do 'mean_im += ...' instead of 'im_out = mean_im + ...' which performs in place addition and have one less variable the size of the image in memory.

Other than that, there's only a few style changes which I can fix myself like 3 space indentation and trailing whitespace.

A minor note on the use of mercurial: you made use of a branch named new-wiener2 but a bookmark was better.  Mercurial bookmarks are akin to git branches.  Mercurial branches are something else without an equivalent on git.  Mercurial branches are meant for long term development, such as stable and development branches.  For short things like development a new feature, an extra head on an existing branch would be better suited, and a bookmark just puts a name to that extra head.

Carnë Draug <carandraug>
Group Member
Thu 25 May 2017 09:06:19 PM UTC, comment #6: 

Testing new version:


>> test wiener2
PASSES 23 out of 23 tests


Avinoam Kalma <avinoam>
Group Member
Thu 25 May 2017 07:48:18 PM UTC, comment #5: 
Hartmut <hardy>
Thu 25 May 2017 05:38:11 PM UTC, comment #4: 


Looks very good.
The ball is now in Carne's hands :-)

Avinoam Kalma <avinoam>
Group Member
Thu 25 May 2017 08:56:45 AM UTC, comment #3: 

Thanks for reviewing, Avinoam.

I've done some changes to the file:

  • missing space: done
  • word "equation": done
  • include book reference into help string: I haven't done this, yet. It feels like inflating the user documentation too much. But I will leave this decision to Carne.
  • incompatibility with incorrect input nhood=[-5 0]: Not done. The resulting error message is due to mean2.m being called with an empty matrix as input. I will try to nail down this small incompatibility (in a possibly different bug report), but I think there shouldn't be a workaround in wiener2.m.


See the resulting (additional) changeset here:
https://bitbucket.org/hgimpel/octave-image/commits/b4e31442fb5d9ff24bc72878776c87026dde93e4

PS: I've just realised (due to Bitbuckets Octave syntax highlighting) that I should change my code to always use double quotes for strings. See here: https://bitbucket.org/hgimpel/octave-image/commits/c78b75bc3861dc69ce00a2775bfaed88b6138159?at=new-wiener2

Hartmut <hardy>
Wed 24 May 2017 06:10:23 PM UTC, comment #2: 

Looks very good, passes all the tests, and even has a nice demo.

I can only remark on a few typos:

  • Missing space in line 119.
  • I would change "formulas" to "equations" in line 46.
  • lines 43-46 should be part of the user documentation.
  • Minor incompatibility in a corner case: wiener2 (I, [-5 0]) returns zeros in Matlab, and does not issue an error. I do not know if you should fix this, because it is really an error.


I recommend to include wiener2 in image package.

Thanks :-)

Avinoam Kalma <avinoam>
Group Member
Wed 24 May 2017 06:47:12 AM UTC, comment #1: 

Here is the very same function (wiener2.m V1) as file attachment. (I don't fully trust the bitbucket, yet...)

(file #40774)

Hartmut <hardy>
Tue 23 May 2017 09:18:18 PM UTC, original submission:  

I've written the function "wiener2.m" for the image package.

This function applies an adaptive noise reduction filter on 2D grayscale images. It is Matlab compatible. The algorithm is taken from the book that is cited on the Mathworks help page for this function.

To make reviewing and commenting easier, I've created my first own bitbucket repository. You can find the new function there:

https://bitbucket.org/hgimpel/octave-image/commits/9af48e1f6eaa5fcafe094f6c2f67a519874ab12c

Please let me know any comments or feedback on this.

Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #40774:  wiener2 (V1).m added by hardy (10KiB - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by hardy (Submitted the item)
  • -email is unavailable- added by hardy
  • -email is unavailable- added by hardy
  •  

    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 logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-12-20 carandraug StatusNone Done
        Open/ClosedOpen Closed
    2017-05-24 hardy Attached File- Added wiener2 (V1).m, #40774
    2017-05-23 hardy Carbon-Copy- Added carandraug
        Carbon-Copy- Added avinoam

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code