patchGNU Octave - Patches: patch #10315, [octave forge] (image) new...

 
 

patch #10315: [octave forge] (image) new functions imgaussfilt and imboxfilt

Submitter:  None
Submitted:  Wed 15 Feb 2023 07:25:07 AM UTC
   
 
Category:  Forge : new function Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Originator Email:  -email is unavailable-
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 23 Mar 2023 08:43:21 PM UTC, comment #15: 

Thanks, @Hartmut, for the major cleanup.
Closing as done. Congratulations.

Avinoam Kalma <avinoam>
Group Member
Thu 23 Mar 2023 03:42:39 PM UTC, comment #14: 

Some more changes on imboxfilt:

https://hg.code.sf.net/p/octave/image/rev/bc8fe419df11

I have also added a (Matlab compatible) unit test here, to check the calculations results with asymmetric filters. The code behaved correct, already. Includes also some more style fixes.

@Avinoam: From my side, the "clean up" of these two new functions is finished, for now. If you want to, you can CLOSE this issue.

Hartmut <hardy>
Thu 23 Mar 2023 02:11:53 PM UTC, comment #13: 

You were right, with seeing trouble with the code mentioned in comment #5. I have now fixed this and added a (Matlab compatible) unit test for it:

https://hg.code.sf.net/p/octave/image/rev/eef8356199f3

The code fix is different than proposed in comment #5 because the names of the variables are very counter-intuitive. But now the calculations results are correct.

Hartmut <hardy>
Sat 11 Mar 2023 01:26:32 PM UTC, comment #12: 

All repo changes mentioned in comment #11 look alright to me. Thanks, Avinoam.

Hartmut <hardy>
Sat 11 Mar 2023 10:17:24 AM UTC, comment #11: 
Avinoam Kalma <avinoam>
Group Member
Wed 08 Mar 2023 08:19:49 AM UTC, comment #10: 

@Avinoam: Let's keep this report open, so we can have any necessary discussion here.

Hartmut <hardy>
Wed 08 Mar 2023 06:05:55 AM UTC, comment #9: 

Thanks, @Hartmut, and thanks Sarah Tiefert and Johannes Wirbser, for adding imgaussfilt and imboxfilt to Octave.

Should we close this report, or keep it until we complete the code review?

Avinoam Kalma <avinoam>
Group Member
Tue 07 Mar 2023 10:25:34 PM UTC, comment #8: 

Thanks for your feedback @Avinoam.

I have now pushed these two original new m-files to the default branch of the image respository:

   https://hg.code.sf.net/p/octave/image/rev/0f5d30c89d57
   https://hg.code.sf.net/p/octave/image/rev/d9ff3e88b6c6

And I have also pushed the suggested changes mentioned in comment #2, comment #3 and comment #4.

   https://hg.code.sf.net/p/octave/image/rev/b553c87c19a9
   https://hg.code.sf.net/p/octave/image/rev/6272f86c51af

I have not yet changed anything regarding comment #5 because I am  unsure if this would be correct, at all.

Once I find the time, I plan to do more clean up of these two fresh m-files.

Hartmut <hardy>
Mon 06 Mar 2023 09:52:02 PM UTC, comment #7: 

Thanks @Hartmut,
In my opinion, we can push these two functions and then make the changes.

Avinoam Kalma <avinoam>
Group Member
Sun 26 Feb 2023 12:36:47 PM UTC, comment #6: 

@Sarah Tiefert and Johannes Wirbser (original code authors): Please let me know if you are willing to check Avinoam's comments and adapt your code contribution accordingly and yourself.

@Avinoam: Please try to decide if the two already supplied m-files are generally acceptable for inclusion into the image package and let us know your opinion, here.

If Avinoam has no general objections against inclusion of this two m-file into the image repository, and if we do not get an answer from the original authors during the next couple of weeks, I would then suggest to push these two m-files unchanged under the names of their two authors. Afterwards we should then do code corrections in seperate commits under our names.

Hartmut <hardy>
Tue 21 Feb 2023 09:23:53 PM UTC, comment #5: 

In the code:


function [fil1, fil2] = create_gaussfilter (filter_size, sigma)
  fil1 = gauss (sigma(1), filter_size(2));
  fil2 = gauss (sigma(2), filter_size(1))';
  fil1 /= sum (fil1);
  fil2 /= sum (fil2);

endfunction


I think that it should be:

  fil1 = gauss (sigma(1), filter_size(1));
  fil2 = gauss (sigma(2), filter_size(2))';


There should be also a relevant test.

Avinoam Kalma <avinoam>
Group Member
Sun 19 Feb 2023 10:11:35 PM UTC, comment #4: 


And the last one for today:

normalizationfactor -> normalization factor (not the parameter)

filtermatrix ->  filter matrix

Avinoam Kalma <avinoam>
Group Member
Sun 19 Feb 2023 10:09:39 PM UTC, comment #3: 

Another problem:

## Always uses convolution based filtering."

but this contradicts:

## determine which filtering method should be used.
## this is based on our internal testing of the algorithms

Avinoam Kalma <avinoam>
Group Member
Sun 19 Feb 2023 09:29:36 PM UTC, comment #2: 

Starting reviewing

1. All the test pass
2. The error lines


    error ("imboxfilt: Img needs to be an integer matrix");
    error ("imgaussfilt: Image (img) needs to be a integer matrix");


are probably wrong, because the Img should be an image, but not necessarily an integer matrix

Avinoam Kalma <avinoam>
Group Member
Wed 15 Feb 2023 02:29:42 PM UTC, comment #1: 

Thank you for your code contributions to the Octave image package. I will add another image package maintainer, Avinoam, for reviewing them.

Hartmut <hardy>
Wed 15 Feb 2023 07:25:07 AM UTC, original submission:  


Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #54358:  imgaussfilt.m added by None (19KiB - text/plain)
file #54359:  imboxfilt.m added by None (18KiB - text/plain)

 

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 (Posted a comment)
  • -email is unavailable- added by None (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 logged-in users can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-03-23 avinoam StatusIn Progress Done
        Open/ClosedOpen Closed
    2023-03-08 avinoam CategoryNone Forge : new function
        StatusNone In Progress
    2023-02-19 avinoam SummaryAdding imgaussfilt and imboxfilt to octave [octave forge] (image) new functions imgaussfilt and imboxfilt
    2023-02-15 None Attached File- Added imgaussfilt.m, #54358
        Attached File- Added imboxfilt.m, #54359

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code