bugGNU Octave - Bugs: bug #44100, image package: regionprops could...

 
 

bug #44100: image package: regionprops could easily deliver more properties

Submitted by:  Hartmut <hardy>
Submitted on:  Tue 27 Jan 2015 08:49:53 PM UTC  
 
Category:  Octave Forge Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  other Operating System:  Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Sat 28 Feb 2015 05:00:08 PM UTC, comment #16:

Done http://hg.code.sf.net/p/octave/image/rev/7cf22a478792

Carnë Draug <carandraug>
Project Member
Fri 27 Feb 2015 07:51:02 PM UTC, comment #15:

Thanks for the hints to the Octave coding style, I'll try to follow them better, next time.

After reading your explanation about copying my code out of the textboxes and not using the supplied changeset file, I've double checked your changeset on the repository. And I've noticed that you (which is logical now) have not used my supplied help texts (from my changeset), but created new ones.

Your new help texts are alright as well. But I found my new help texts (in my changeset) more precise. This in only to inform you that those (my) new help texts are also available. You can stick to your ones, or update the repository with my help texts if you like them better (as I do...).

Hartmut <hardy>
Fri 27 Feb 2015 07:37:30 PM UTC, comment #14:

I'm sorry. You pasted the code on the comment and I didn't notice the attached file. I copied the code from your comment into the file and made the commit with your name.

You did everything right, I just didn't notice the file.

Would be nice if you could follow the coding guidelines a bit better next time, i.e., use spaces between function names and brackets but not after variables and brackets used to index, e.g.:

Since we use () both for indexing and calling functions, this helps distinguish them when reading. Also, no trailing whitespace. I also grouped the tests together since they were all sharing the sam einput.

Carnë Draug <carandraug>
Project Member
Fri 27 Feb 2015 06:28:43 PM UTC, comment #13:

Thanks for changing this so fast.

Yes, I will check this "ew" email address regularly. But nevertheless I like to keep email addresses separate for seperate purposes. Most probably the "old" one, used mainly for forum logins, will be phased out at some time in the future.

I have already cloned the image repo. And the changeset I supplied already was a proper mercurial changeset, as described in your web link. Is there anything I should do differently the next time?

Hartmut <hardy>
Fri 27 Feb 2015 05:13:03 PM UTC, comment #12:

It is done.

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

But please note that this is not meant to be the email for contact for help. It will probably only be used for copyright issues (which will hopefully not arise), important stuff so do not use an email that you do not check.

Also, I used the other email as author of the changeset. I changed the address on the source but on the author of the changeset. Would bw great if you cloned the image package repo and submitted a cset file next time. These are the instructions for Octave core but for the image package it's the same thing https://www.gnu.org/software/octave/doc/interpreter/Basics-of-Generating-a-Changeset.html

Carnë Draug <carandraug>
Project Member
Fri 27 Feb 2015 03:46:39 PM UTC, comment #11:

Thanks for integrating my patch into the image package. This feels exciting for me :)

One last wish: Could you please change my email address in the source code to the one that I've supplied within the patch? You' have used the one I had registered in Savanna, but this one wasn't ment to be public. (I've just changed my Savanna email adress as well to avoid this in the future.) Thanks!

Hartmut <hardy>
Fri 27 Feb 2015 01:04:26 PM UTC, comment #10:

Thank you for the patch. I have pushed it as http://hg.code.sf.net/p/octave/image/rev/d54065343186

A good improvement would be to avoid the basic computations for each property. For example, when computing Orientation and Eccentricity, it could reuse the computed values of Major and Minor axis length instead of computing them again.

Carnë Draug <carandraug>
Project Member
Fri 27 Feb 2015 08:30:51 AM UTC, comment #9:

The "old" orientation option (current version in the repository) was not Matlab compatible, because the output values were not between -90 and 90 degrees. Furthermore the old version probably gave slightly different return values, because "orientation" means the orientation of the "ellipse with the same centralised second moments", and this ellipse wasn't calculated properly before. So now all 4 properties that correspond to this special ellipse, are calculated consistently and the results are Matlab compatible.

Hartmut <hardy>
Fri 27 Feb 2015 12:08:08 AM UTC, comment #8:

Why does you patch reimplements the orientation option? Is it fixing anything there? The orientation is not commented on the repository

http://hg.code.sf.net/p/octave/image/file/c5c645f29103/inst/regionprops.m

Carnë Draug <carandraug>
Project Member
Wed 25 Feb 2015 12:06:17 PM UTC, comment #7:

I've found someone who checked with Matlab, that those 4 new properties are really Matlab compatible now: and they are :)

And I've put all the code below into a hg changeset (see attached file): the calculation of the 4 properties, new help texts and tests.

Since this is only my second changset I supply, please feel free to correct any unusual formatting etc.

Can someone now integrate this into the image package?

(file #33182)

Hartmut <hardy>
Wed 04 Feb 2015 12:29:23 PM UTC, comment #6:

Here is a new version for the 4 "ellipse" properties in the regionprops.m file. (They are slightly improved with respect to the last version, now they are more accurate for very small regions.)

As you can see, I've also added the Orientation feature to my code. Those 4 properties are consistent now. And I think they should be Matlab compatible as well. (It may be worth checking the results of the tests with a real Matlab nevertheless. I don't have access to it for the next months.)

And here are a few tests for those 4 new features:

Hartmut <hardy>
Sat 31 Jan 2015 06:25:08 PM UTC, comment #5:

Could you provide tests for these features? If you take a look at the bottom of the regionprops.m file, you will see a bunch of %! blocks. These are the tests and ensure that future changes will not break existing functionality.

Can you add new tests, at least one for each property?

Carnë Draug <carandraug>
Project Member
Fri 30 Jan 2015 11:56:26 PM UTC, comment #4:

Sorry for the confusion. Here is a shorter version that does the very same:

Hartmut <hardy>
Fri 30 Jan 2015 11:38:55 PM UTC, comment #3:

Yes, I think I can now confirm that the old (commented out) versions were NOT Matlab compatible.

Here are my new proposals for the mentioned additional properties of regionprops.m:

As said before, I could not 100% benchmark the results as compared to Matlab. But I do get a very good match with the resulting image in this Matlab-Tutorial: http://blogs.mathworks.com/steve/2010/07/30/visualizing-regionprops-ellipse-measurements/

Hartmut <hardy>
Wed 28 Jan 2015 08:08:55 AM UTC, comment #2:

Sorry, I didn't know about the long history of those features.

Currently I do not have access to Matlab, so I won't be of any help for clarifying compatibility issues. Once I regain access to Matlab at the end of this year I could try to work on this.

Hartmut <hardy>
Tue 27 Jan 2015 10:02:56 PM UTC, comment #1:

If I remember correctly (it was a long time ago, I may not be), their results were not Matlab compatible. They would eventually have to be replaced and cause a bunch of trouble because we would not be backwards compatible.

Could you confirm test the above (that the commented computations are not Matlab compatible)? If they are compatible, could you provide with a patch with tests for them? If not, would you be willing to implement compatible methods?

Carnë Draug <carandraug>
Project Member
Tue 27 Jan 2015 08:49:53 PM UTC, original submission:

In the source code file "regionprops.m" of the release image-2.2.2 the properties "orientation", "majoraxislength" and "minoraxislength" are included but commented out (and not listed in the all_props list at the beginning of the file). I found their output reasonable and useful, would it be possible to comment them in for the next release of the image package?

Additionally the property "eccentricity" could be easily implemented: with a=majoraxislength and b=minoraxislength it is definded as eccentricity=sqrt(1-(b/a)^2) .

Hartmut <hardy>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #33182:  ellipsprops.diff added by hardy (8KiB - 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)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-02-27 carandraug StatusNone => Fixed
        Open/ClosedOpen => Closed
        Release3.8.2 => other
    2015-02-25 hardy Attached File- => Added ellipsprops.diff, #33182

    Back to the top


    Powered by Savane 3.4