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

 
 

patch #9360: image package: new function otsuthresh.m

Submitter:  Avinoam Kalma <avinoam>
Submitted:  Thu 25 May 2017 05:32:59 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

Fri 21 Dec 2018 03:23:13 AM UTC, comment #15: 

I have pushed this now.

Carnë Draug <carandraug>
Group Member
Sat 01 Dec 2018 10:25:46 PM UTC, comment #14: 

Actually, nevermind my comment. isnan returns true for NA. Although I guess you could just skip a lot of the checks if isinteger returns true.

Carnë Draug <carandraug>
Group Member
Sat 01 Dec 2018 09:56:34 PM UTC, comment #13: 

Thanks for the reviews (comment #11 & comment #12).
I will prepare a revised version

Avinoam Kalma <avinoam>
Group Member
Sat 01 Dec 2018 06:34:33 PM UTC, comment #12: 

Other than Hartmut comments, looks fine to me. Small nitpick. Instead of checking for Inf, NaN, and < 0 (and forgetting NA), you can just do:


! all (hist >= 0 | hist < Inf)


Also, are we just checking for sparse because Matlab does not support it? I think we lose nothing by letting it pass. What do we care if an histogram is internally stored as a sparse array?

Carnë Draug <carandraug>
Group Member
Wed 28 Nov 2018 09:54:39 PM UTC, comment #11: 

I have had a look at the new file version (V3):

  • my issues from comment #8 seem to be fixed (there doesn't seem to be any trouble with imcast...)
  • Are all those tests Matlab compatible? (I have not checked this.)
  • The following (Matlab compatible) input classes are currently not tested: int64, uint32, uint64. Do they work? (then let's test them) Or dont' they work for a good reason?(then let's document this in the help string). My short testing showed that they seem to work well, if that's true then please just add a test for each possible input class.


Hartmut <hardy>
Tue 27 Nov 2018 07:32:09 PM UTC, comment #10: 

Attached otsuthresh_V3.m
please review

(file #45540)

Avinoam Kalma <avinoam>
Group Member
Sun 25 Nov 2018 05:31:43 PM UTC, comment #9: 

Thanks for the review.
I will fix these comments, and send another patch for review.

Avinoam Kalma <avinoam>
Group Member
Sat 24 Nov 2018 08:16:18 PM UTC, comment #8: 

I have had another closer look at this patch (file #40868). Here are my comments:

  • There is a wrong character in the first lines, the last character in the first name of Carne.
  • You could add a check to reject sparse inputs, Matlab claims to reject them.
  • Several tests do not do what they pretend to do! For example this code "H = zeros(1,2,'int32'); H = [100 200];" does NOT give a vector of class uint32, but a vector of class double! So most of the integer input classes are untested.
  • I have tried all integer input classes that Matlab claims to support. The following do NOT work: int8, int32, int64, uint32, uint64. Most of them don't work because "imcast" cannot deal with them.


@Avinoam: Could you fix this?

To get full coverage of all Matlab compatible input classes we would probably need to change imcast as well. Or can we live with an otsuthresh, that can only deal with the most common integer classes (the following already work: uint8, uint16, int16).

Hartmut <hardy>
Wed 14 Nov 2018 09:01:26 AM UTC, comment #7: 

Please review

Avinoam Kalma <avinoam>
Group Member
Tue 06 Jun 2017 09:00:33 PM UTC, comment #6: 

Matlab otsuthresh can get a column or a raw vector, but not
a matrix.

Attached a better version, with more tests.
All the tests will pass, after fixing two bugs in graythresh, see bug #45333.

There is still a "divide by zero" warning. It can be fixed
also in graythresh.

(file #40868)

Avinoam Kalma <avinoam>
Group Member
Mon 05 Jun 2017 11:04:44 AM UTC, comment #5: 

Could you also confirm whether Matlab requires a column or array vector? And what happens if you pass a matrix? Would be nice to pass a matrix, one histogram per column, and get back an array of threshold values. This would allow to threshold an array of images in one go.

Carnë Draug <carandraug>
Group Member
Sun 04 Jun 2017 04:44:08 PM UTC, comment #4: 

Thanks for reviewing.
I will add more test, as you suggested.

Avinoam Kalma <avinoam>
Group Member
Sun 04 Jun 2017 11:54:35 AM UTC, comment #3: 

I have made some changes to it:

https://bitbucket.org/carandraug/octave-image/commits/1664caa6b2729a50adb47700cbbdf9bb8194f522

When it comes to tests, since the point of this function is to use on processed histograms and to control the number of bins, can you use histograms that are not already used in the graythresh? Things like histograms with 10 bins, all bins with a value of zero, an histogram of length 0 and one, an histogram with values of class integer, an histogram with an Inf, etc?

At the moment, Octave comes with penny.mat which could be used for testing purposes and is very unlikely to change.

Carnë Draug <carandraug>
Group Member
Sat 27 May 2017 07:22:13 AM UTC, comment #2: 

Thanks for reviewing, Hartmut.

A few answers:

  • I will check Matlab results on the test images
  • I have copied the demo from your demo in wiener2 ....
  • Indeed we need more images in Octave for tests and demos. I raise this issue in the maintainers list and quoted you.


Avinoam Kalma <avinoam>
Group Member
Fri 26 May 2017 04:56:19 PM UTC, comment #1: 

Thanks for your contribution!

I've had a look at this new function:

  • This is a wrapper around the already existing functionality of Otsu's threshhold in Octave's graythresh function. So any wrong output values wouldn't be due to this new otsuthresh function anyways. But it would still be nice to have the outputs Matlab compatible. I have not checked the results of the included tests in Matlab, have you? Do they give the same results in Matlab?


  • The included tests all pass.


  • Nice demo included :)



(general remark: It would help to have some simple image files distributed along with the image package. Those could be used for tests and demos more easily. Or are there already some of those, in addition to phantom.m, that I just don't know of?)

I would like to see this new function added to the image package. (To keep up with Matlab changes, this was only introduced in R2016a...)

Hartmut <hardy>
Thu 25 May 2017 05:32:59 PM UTC, original submission:  


Implementing otsuthresh using graythresh.

Please review.

Avinoam Kalma <avinoam>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45540:  otsuthresh_V3.m added by avinoam (7KiB - application/octet-stream)
file #40868:  otsuthresh.m added by avinoam (5KiB - application/octet-stream)
file #40790:  otsuthresh.m added by avinoam (11KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-12-21 carandraug StatusIn Progress Done
        Open/ClosedOpen Closed
    2018-12-01 avinoam StatusNone In Progress
    2018-11-27 avinoam Attached File- Added otsuthresh_V3.m, #45540
    2017-06-06 avinoam Attached File- Added otsuthresh.m, #40868
    2017-05-25 avinoam Attached File- Added otsuthresh.m, #40790
        Carbon-Copy- Added -email is unavailable-
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code