patchGNU Octave - Patches: patch #9252, image package: hough.m (Matlab...

 
 

patch #9252: image package: hough.m (Matlab compatible wrapper around hough_line.cc)

Submitter:  Hartmut <hardy>
Submitted:  Mon 06 Feb 2017 10:04:40 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
   

Sat 18 Feb 2017 12:41:47 AM UTC, comment #5: 

I have pushed your patch. I made two more changes:

1. added an otherwise on the switch block to catch any unknown property
2. made the function error if the user tries to set RhoResolution to anything other than 1.  If a function can't do what the user wants, I think the best coure of action is error and inform the user about it instead of doing something else.

This will be part the next minor release (default branch):

http://hg.code.sf.net/p/octave/image/rev/c8bc315fa273

> We could now move hough_line to the private directory, or maybe only deprecate it for now. (Maybe even deprecate houghtf.m along the way)



hough_line is a an oct file so it can't be moved to private/ (oct files need to be in the architecture dependent directory). And it can't be deprecated either because hough now makes use of it, meaning that calling hough would trigger a deprecated warning. You would have to rename hough_line to __hough__ (the double underscores mark it as private for the users), have hough call this new __hough__, make a new hough_line function that just calls __hough__, and then deprecate hough_line.

> In my original patch I also had a correction of the help text of hough_line.cc, this would be useful to keep. (It might even be applied to the stable branch already, it's a correction of the documentation.)

You are right, this could have been committed to the stable branch but I got distracted and pushed to the default branch instead. If there is a patch release, I will graft it into stable.


http://hg.code.sf.net/p/octave/image/rev/81e480bd51b6

Carnë Draug <carandraug>
Group Member
Mon 13 Feb 2017 07:27:32 PM UTC, comment #4: 

Thanks for reviewing my patch.

I have checked the Matlab behavior of hough.m. And yes, it really does convert any BW input into a logical straight away. Any value that is not zero will be counted as a one. Especially your new test  (using the values [0.7 0.2 5]) passes fine in Matlab.

So I am fine with all your changes. I would be happy to see this pushed to the forge image repository and be used in the next release of the image package.

Some notes that come to my mind (which I haven't seen in the bitbucket repo so far):

  • We could now move hough_line to the private directory, or maybe only deprecate it for now. (Maybe even deprecate houghtf.m along the way)
  • In my original patch I also had a correction of the help text of hough_line.cc, this would be useful to keep. (It might even be applied to the stable branch already, it's a correction of the documentation.)
Hartmut <hardy>
Sun 12 Feb 2017 12:08:22 AM UTC, comment #3: 

Ok. I just wanted to make sure that the missing houghlines and houghpeaks should be dependent on hough rather than the other way around.

After changing the line breaks from dos to unix

https://bitbucket.org/carandraug/octave-image/commits/6d07c845470fd438e03cedb7b56d487b9cad4463

I made this changes (see inline comments)

https://bitbucket.org/carandraug/octave-image/commits/bae473473e18dbcee6fb570bb85cec983ca9d45b

The only important change I made is about handling of non binary input. Can you check Matlab behaviour? It's very rare for Matlab to actually check, it usually just converts it to logical.

If that's ok with you, I will push that to Octave Forge.

Carnë Draug <carandraug>
Group Member
Tue 07 Feb 2017 09:42:25 PM UTC, comment #2: 
  • hough_line.cc (Octave):


** I have no objections to deprecating hough_line.cc once we have the (mostly) Matlab compatible hough.m present. The new hough.m can do everything that previously did hough_line.cc, plus hough.m has some additional "convinience options" like the Matlab version has.

** The old hough_line.cc does need to stay inside the image package, though. Probably as a private function. Because the new hough.m is mostly a wrapper script around hough_line.cc. The real computational work is still done by hough_line.cc

  • houghpeaks.m and houghlines.m (Matlab):


** Those Matlab functions are "convenience" scripts, that do useful post-processing of the results of hough.m. In the Octave image package we currently do not have any compatible pendants that I know of. (So those would probably stay missing functions in Octave.)

** An Octave version of houghpeaks.m might be doable by writing a wrapper script around our present immaximas.m script.

** An Octave version of houghlines.m would be a fully new implementation of this task in Octave. It shouldn't be too big a job, but I don't know of any alreay existing Octave script that already aims at the same task.

  • houghtf.m  (Octave):
    • Our (Octave-only) current function houghtf.m is very short and simple. Calling it with one (or without any) option redirects the call to hough_line.cc. And calling it with the other options redirects the call to hough_circle.m. If we deprecate hough_line.cc then we might also deprecate houghtf.m


  • hough_circle.m (Octave) and imfindcircles.m (Matlab):
    • Our hough_circle.m script does the circular Hough transform. This is computationally independent of the linear Hough transform in hough_line.cc and hough.m. So hough_circle.m would need to stay. In order to also substitute it with a Matlab compatible function, one would need to implement imfindcircles.m in Octave (it also does the circular Hough transform).



Hartmut <hardy>
Tue 07 Feb 2017 02:23:09 PM UTC, comment #1: 

When his happens, the previous hough_line function should be deprecated. Can hough_line do anything that hough cannot? How does it relate with houghlines. How would that be implemented?

Carnë Draug <carandraug>
Group Member
Mon 06 Feb 2017 10:04:40 PM UTC, original submission:  

In the attached PATCH (V1) you find a new m-file script hough.m.

This is a (mostly) Matlab compatible wrapper around the already existing hough_line.cc. It also adds some tests to this and slightly corrects the documentation.

Let me know any feedback on this patch to the image package.

Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #39685:  new_hough_V1.patch added by hardy (11KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -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
    2017-02-18 carandraug StatusNone Done
        Open/ClosedOpen Closed
    2017-02-06 hardy Attached File- Added new_hough_V1.patch, #39685
        Carbon-Copy- Added avinoam
        Carbon-Copy- Added carandraug

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code