bugGNU Octave - Bugs: bug #41186, image: fspecial disk is not matlab...

 
 

bug #41186: image: fspecial disk is not matlab compatible

Submitter:  Carnë Draug <carandraug>
Submitted:  Fri 10 Jan 2014 01:16:04 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * other Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 06 Mar 2014 07:14:56 PM UTC, comment #6: 


cset f1cf02800a87




This problem has been fixed in the development version. The fix
will be available in the next major software release. Thank you
for your bug report.

Carnë Draug <carandraug>
Group Member
Wed 05 Mar 2014 09:04:51 PM UTC, comment #5: 

Same as the previous version, but compliant with coding style and using asserts in the test. Also includes a new test. This is a diff against rev 871.

(file #30784)

Joonas Lipping <alcedine>
Sat 01 Mar 2014 09:55:46 PM UTC, comment #4: 

New version. In this one, the nested subfunction has been inlined. There is still a loop, but I don't think that there is an efficient way to achieve this functionality without one, due to the fact that the edge of the disk can hit a square in at least three ways: enter horizontally, exit horizontally; enter vertically, exit horizontally; enter horizontally, exit vertically. It's a bit tricky to take all these into account in a simple expression.

I would argue that it is fine to have the loop, as it only iterates along 1/8 of the disk's rim, which means O(radius) squares hit. I tested the running time on radii from 1 to 1000, comparing the revised function to the current one, and the running time multiplier peaks at a little below 12 (somewhere between radii 100 and 200) and seems to asymptotically approach ~5. The new function is faster than the previous one I submitted by a factor of 2 or perhaps a little less.

I added a bit to the documentation at the top of the file about what the function now does. I also edited NEWS. I added a test that checks for a couple different radii that the sum of the elements is approximately 1.

I haven't been able to check if the output corresponds to Matlab exactly, but at least the default range gives the same output within long format accuracy.

(file #30746)

Joonas Lipping <alcedine>
Fri 28 Feb 2014 07:12:25 PM UTC, comment #3: 

I think both the nested function and the loop is ok in this case. Here the nested function is actually using variables from the enclosing function, so the nesting is actually helpful. As for looping, the performance hit is not significant, since the matrix being looped over is generally tiny.

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Fri 28 Feb 2014 05:42:32 PM UTC, comment #2: 

There are two things that should be avoided here:

1) avoid nested subfunctions. Instead, create a subfunction at the end of the file (or if the function is really small, create a function handle).

2) avoid loops. These are not good for performance.

Since it seems you are a GSoC canditate trying to get used to Octave development, I will also suggest some other smaller things:

1) add a test block for this feature. These are the %! blocks at the end of many m files. fspecial does not have them yet but take a look at bwmorph or imcomplement for examples.

2) add a note to the documentation of fspecial() about what the "disk" option. This needs to be documented somewhere.

3) add a note to the NEWS file about this change (note that for a new user, this is just a bug fix but someone using Octave for a long time, this is a backwards incompatible change).

4) some comment on the code about what is going on. The comment for the attachment "Each cell is weighted according to how much of it is covered by the disk" is the most useful, why was it not in the actual code or even in the commit message?

Carnë Draug <carandraug>
Group Member
Thu 27 Feb 2014 10:49:23 PM UTC, comment #1: 

I am adding a comment to comply with the warning on the Short Projects wiki page (for file #30710 below).

I have adjusted the fspecial function 'disk' case. In the (2R+1)x(2R+1) matrix M that is returned, each element corresponds to a square of size 1 on a grid. Let there be a disk of radius R centered in the middle of the square corresponding to element m_{R+1,R+1}. Then m_{ij} = p_{ij}/A, where p_{ij} is the fraction of the corresponding square that is covered by the disk and A = \pi R^2 is the total area of the square.

This appears to give the same values as Matlab's fspecial function, up to "format long" accuracy.

Joonas Lipping <alcedine>
Fri 10 Jan 2014 01:16:04 PM UTC, original submission:  

Octave's fspecial disk shape, returns a disk shaped filter for average, where all have the same value. However, the Matlab disk is slightly different, where the borders are slightly lower (pillbox).


>> a = fspecial ('disk')

a =

         0         0         0    0.0012    0.0050    0.0063    0.0050    0.0012         0         0         0
         0    0.0000    0.0062    0.0124    0.0127    0.0127    0.0127    0.0124    0.0062    0.0000         0
         0    0.0062    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0062         0
    0.0012    0.0124    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0124    0.0012
    0.0050    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0050
    0.0063    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0063
    0.0050    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0050
    0.0012    0.0124    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0124    0.0012
         0    0.0062    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0127    0.0062         0
         0    0.0000    0.0062    0.0124    0.0127    0.0127    0.0127    0.0124    0.0062    0.0000         0
         0         0         0    0.0012    0.0050    0.0063    0.0050    0.0012         0         0         0


Carnë Draug <carandraug>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #30784:  fspecial.diff added by alcedine (5KiB - text/plain - Edited to comply with coding standards. New test. Both now use asserts.)
file #30746:  fspecial.diff added by alcedine (5KiB - text/plain - New version, with inlined AUC function, tests and clarified in documentation.)
file #30710:  fspecial.diff added by alcedine (3KiB - text/plain - Modified fspecial for the disk to mimic Matlab behavior. Each cell is weighted according to how much of it is covered by the disk x^2 + y^2 <= radius^2.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by alcedine (Updated the item)
  • -email is unavailable- added by carandraug (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-03-06 carandraug StatusNone Fixed
        Open/ClosedOpen Closed
    2014-03-05 alcedine Attached File- Added fspecial.diff, #30784
    2014-03-01 alcedine Attached File- Added fspecial.diff, #30746
    2014-02-26 alcedine Attached File- Added fspecial.diff, #30710

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code