Contributing code

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Contributing code

Roberto Metere
Dear Octave maintainers,
    I would like to contribute octave-forge repository by adding a missing function "strel" to the package image, which I have implemented for the most used arguments. Some cases don't work yet (exiting with a "not yet implemented" message), but I surely will keep on work.

My SourceForge name is trinitrina
I have attached my contribution.

I may imagine you have some rules for coding, like no tabs, custom comments, etc.
Obviously I want to change my code to satisfy those rules, may someone help me to begin?

Best regards,
    Roberto Metere


strel.m (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Contributing code

Juan Pablo Carbajal-2
On Fri, Dec 14, 2012 at 12:36 PM, Roberto Metere <[hidden email]> wrote:

> Dear Octave maintainers,
>     I would like to contribute octave-forge repository by adding a missing
> function "strel" to the package image, which I have implemented for the most
> used arguments. Some cases don't work yet (exiting with a "not yet
> implemented" message), but I surely will keep on work.
>
> My SourceForge name is trinitrina
> I have attached my contribution.
>
> I may imagine you have some rules for coding, like no tabs, custom comments,
> etc.
> Obviously I want to change my code to satisfy those rules, may someone help
> me to begin?
>
> Best regards,
>     Roberto Metere
>

Roberto,

Thank you fro the contribution!

the best way to get to know the standard is to look at other files
already in the image package.
You can also check the guidelines here
http://www.gnu.org/software/octave/doc/interpreter/General-Guidelines.html

Also follow this thread
https://mailman.cae.wisc.edu/pipermail/octave-maintainers/2010-November/021419.html
Reply | Threaded
Open this post in threaded view
|

Re: Contributing code

Carnë Draug-2
In reply to this post by Roberto Metere
On 14 December 2012 11:36, Roberto Metere <[hidden email]> wrote:

> Dear Octave maintainers,
>     I would like to contribute octave-forge repository by adding a missing
> function "strel" to the package image, which I have implemented for the most
> used arguments. Some cases don't work yet (exiting with a "not yet
> implemented" message), but I surely will keep on work.
>
> My SourceForge name is trinitrina
> I have attached my contribution.
>
> I may imagine you have some rules for coding, like no tabs, custom comments,
> etc.
> Obviously I want to change my code to satisfy those rules, may someone help
> me to begin?

Hey

that's great. I wanted this function in the package for quite some
time. My impression is that strel should returns a strel object. Your
code just returns the matrix of the SE. We don't have classdef but it
seems to me that strel could be implemented just fine with the old
style of objects. See
http://www.gnu.org/software/octave/doc/interpreter/Object-Oriented-Programming.html

Other than that, I didn't had a real look at the actual code, just at
how it looks like and about guidelines, here's some:

1) help text in texinfo. See http://wiki.octave.org/Help_text
2) 2 spaces instead of tabs
3) add a closing endfunction, and indent the whole function block
4) no need for [ ] around strings. The brackets are not necessary in
"error (['error message'])"
5) all your usage lines can be replaced "if (.....) print_usage(); endif
6) don't be afraid of whitespace. General rule is space before
parentheses if it's a function call, no space if it's indexing a
variable.
7) use double quotes
8) use specific end block (endif, endfor, endswitch, etc)

It may be better to submit to the feature request tracker at
https://sourceforge.net/p/octave/feature-requests/ Things like this,
tend to get lost too easily in the mailing list.

Take a look at the following functions for an idea on things usually
are in the image package:

https://sourceforge.net/p/octave/code/11485/tree/trunk/octave-forge/main/image/inst/imnoise.m
https://sourceforge.net/p/octave/code/11485/tree/trunk/octave-forge/main/image/inst/cmunique.m

Carnë
Reply | Threaded
Open this post in threaded view
|

Re: Contributing code

Roberto Metere
In reply to this post by Juan Pablo Carbajal-2
On 12/14/2012 12:58 PM, Juan Pablo Carbajal wrote:
On Fri, Dec 14, 2012 at 12:36 PM, Roberto Metere [hidden email] wrote:
Dear Octave maintainers,
    I would like to contribute octave-forge repository by adding a missing
function "strel" to the package image, which I have implemented for the most
used arguments. Some cases don't work yet (exiting with a "not yet
implemented" message), but I surely will keep on work.

My SourceForge name is trinitrina
I have attached my contribution.

I may imagine you have some rules for coding, like no tabs, custom comments,
etc.
Obviously I want to change my code to satisfy those rules, may someone help
me to begin?

Best regards,
    Roberto Metere

Roberto,

Thank you fro the contribution!

the best way to get to know the standard is to look at other files
already in the image package.
You can also check the guidelines here
http://www.gnu.org/software/octave/doc/interpreter/General-Guidelines.html

Also follow this thread
https://mailman.cae.wisc.edu/pipermail/octave-maintainers/2010-November/021419.html

Juan,
    As first, thank you for your previous mail.
I adapted my code to respect general guidelines and added two initial test cases at the end, following what I have seen in other files in "image" package.

I think it's time for my first "Initial commit to SVN."
How can I do? do I need permission, don't I?

What I've just done:
  1. I launched "svn checkout http://svn.code.sf.net/p/octave/code/trunk/octave-forge/"
  2. Added my file to main/image/inst/
Reply | Threaded
Open this post in threaded view
|

Re: Contributing code

Juan Pablo Carbajal-2
On Sat, Dec 15, 2012 at 5:00 PM, Roberto Metere <[hidden email]> wrote:

> On 12/14/2012 12:58 PM, Juan Pablo Carbajal wrote:
>
> On Fri, Dec 14, 2012 at 12:36 PM, Roberto Metere <[hidden email]> wrote:
>
> Dear Octave maintainers,
>     I would like to contribute octave-forge repository by adding a missing
> function "strel" to the package image, which I have implemented for the most
> used arguments. Some cases don't work yet (exiting with a "not yet
> implemented" message), but I surely will keep on work.
>
> My SourceForge name is trinitrina
> I have attached my contribution.
>
> I may imagine you have some rules for coding, like no tabs, custom comments,
> etc.
> Obviously I want to change my code to satisfy those rules, may someone help
> me to begin?
>
> Best regards,
>     Roberto Metere
>
> Roberto,
>
> Thank you fro the contribution!
>
> the best way to get to know the standard is to look at other files
> already in the image package.
> You can also check the guidelines here
> http://www.gnu.org/software/octave/doc/interpreter/General-Guidelines.html
>
> Also follow this thread
> https://mailman.cae.wisc.edu/pipermail/octave-maintainers/2010-November/021419.html
>
> Juan,
>     As first, thank you for your previous mail.
> I adapted my code to respect general guidelines and added two initial test
> cases at the end, following what I have seen in other files in "image"
> package.
>
> I think it's time for my first "Initial commit to SVN."
> How can I do? do I need permission, don't I?
>
> What I've just done:
>
> I launched "svn checkout
> http://svn.code.sf.net/p/octave/code/trunk/octave-forge/"
> Added my file to main/image/inst/

Roberto,

Yes you need permission, but the one taking that decision is Carnë, he
is the boss ;)
It would be good to know if you pretend to stay tuned with Octave and
help maintaining the packages (image in your case, for example). If
not i better that you follow the one-time contribution schema
http://wiki.octave.org/Contributing_to_the_development_of_packages/modules

btw, where is your updated code?
Reply | Threaded
Open this post in threaded view
|

Re: Contributing code

Roberto Metere
On 12/15/2012 05:46 PM, Juan Pablo Carbajal wrote:
On Sat, Dec 15, 2012 at 5:00 PM, Roberto Metere [hidden email] wrote:
On 12/14/2012 12:58 PM, Juan Pablo Carbajal wrote:

On Fri, Dec 14, 2012 at 12:36 PM, Roberto Metere [hidden email] wrote:

Dear Octave maintainers,
    I would like to contribute octave-forge repository by adding a missing
function "strel" to the package image, which I have implemented for the most
used arguments. Some cases don't work yet (exiting with a "not yet
implemented" message), but I surely will keep on work.

My SourceForge name is trinitrina
I have attached my contribution.

I may imagine you have some rules for coding, like no tabs, custom comments,
etc.
Obviously I want to change my code to satisfy those rules, may someone help
me to begin?

Best regards,
    Roberto Metere

Roberto,

Thank you fro the contribution!

the best way to get to know the standard is to look at other files
already in the image package.
You can also check the guidelines here
http://www.gnu.org/software/octave/doc/interpreter/General-Guidelines.html

Also follow this thread
https://mailman.cae.wisc.edu/pipermail/octave-maintainers/2010-November/021419.html

Juan,
    As first, thank you for your previous mail.
I adapted my code to respect general guidelines and added two initial test
cases at the end, following what I have seen in other files in "image"
package.

I think it's time for my first "Initial commit to SVN."
How can I do? do I need permission, don't I?

What I've just done:

I launched "svn checkout
http://svn.code.sf.net/p/octave/code/trunk/octave-forge/"
Added my file to main/image/inst/
Roberto,

Yes you need permission, but the one taking that decision is Carnë, he
is the boss ;)
It would be good to know if you pretend to stay tuned with Octave and
help maintaining the packages (image in your case, for example). If
not i better that you follow the one-time contribution schema
http://wiki.octave.org/Contributing_to_the_development_of_packages/modules

btw, where is your updated code?

Juan,
    you're right. I didn't attached in previous mail.

Indeed, I would stay tuned the package image, expecially because my function needs to be completed in the sense of managing all argument types, in order to get a quite full compatibility to that corresponding in MATLAB.

I'm still thinking about the one-time contribution way: in this case I would send updated "versions" of the function(s).

Moreover, should I better adapt my code?



strel.m (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Contributing code

Carnë Draug
In reply to this post by Roberto Metere
On 15 December 2012 16:00, Roberto Metere <[hidden email]> wrote:

> On 12/14/2012 12:58 PM, Juan Pablo Carbajal wrote:
>
> On Fri, Dec 14, 2012 at 12:36 PM, Roberto Metere <[hidden email]> wrote:
>
> Dear Octave maintainers,
>     I would like to contribute octave-forge repository by adding a missing
> function "strel" to the package image, which I have implemented for the most
> used arguments. Some cases don't work yet (exiting with a "not yet
> implemented" message), but I surely will keep on work.
>
> My SourceForge name is trinitrina
> I have attached my contribution.
>
> I may imagine you have some rules for coding, like no tabs, custom comments,
> etc.
> Obviously I want to change my code to satisfy those rules, may someone help
> me to begin?
>
> Best regards,
>     Roberto Metere
>
> Roberto,
>
> Thank you fro the contribution!
>
> the best way to get to know the standard is to look at other files
> already in the image package.
> You can also check the guidelines here
> http://www.gnu.org/software/octave/doc/interpreter/General-Guidelines.html
>
> Also follow this thread
> https://mailman.cae.wisc.edu/pipermail/octave-maintainers/2010-November/021419.html
>
> Juan,
>     As first, thank you for your previous mail.
> I adapted my code to respect general guidelines and added two initial test
> cases at the end, following what I have seen in other files in "image"
> package.
>
> I think it's time for my first "Initial commit to SVN."
> How can I do? do I need permission, don't I?
>
> What I've just done:
>
> I launched "svn checkout
> http://svn.code.sf.net/p/octave/code/trunk/octave-forge/"
> Added my file to main/image/inst/

Hi Roberto

We usually request new contributors for a few contributions before
giving them commit access. I'll happily commit your code and
eventually we will give you access.

I have taken a look at your new code and here's a list of improvements
you could do:

1) matlab strel does not return a logical matrix, its "getnhood" method does.

2) there's other simpler changes that would make the code much simpler
to maintain. As an example take a look at
http://agora.octave.org/snippet/q2hq/ (the first block is yours, the
second is an improvement, the third is what is usually into Octave).
  * too many nested if blocks. You can use ! before the test and elseif
  * while it's good to be specific on the error message, on this case
should be enough to place them all under the same error message
  * don't use size (x) == [1 1] to check if it's a scalar. Use isscalar()
  * don't use uint8 to check if a number is an integer, that would
place a limit on 255. Use fix()
  * don't use [ ] around quotes. They are not necessary. If you want
to add a variable to it, use the syntax "error ("the thing %s is not
implemented", varname)"
  * don't use "ones (x, x, "logical")", use "true (x)" instead
  * since we already made the check that arg1 is a single positive
integer, you can use it directly, no need for uint8()
  * the error message I used at the end is just to be standard

Such change could be easily applied to the rest of your code.

Again, please make further code submission to the feature request
tracker at https://sourceforge.net/p/octave/feature-requests/ They are
less likely to get lost there and the maintainers mailing list is not
the place to discuss this things.

Carnë