[jira] [Created] (MATH-851) Add convolution

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

[jira] [Created] (MATH-851) Add convolution

AD_LB (Jira)
Clemens Novak created MATH-851:
----------------------------------

             Summary: Add convolution
                 Key: MATH-851
                 URL: https://issues.apache.org/jira/browse/MATH-851
             Project: Commons Math
          Issue Type: New Feature
            Reporter: Clemens Novak
            Priority: Minor


I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Updated] (MATH-851) Add convolution

AD_LB (Jira)

     [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Clemens Novak updated MATH-851:
-------------------------------

    Attachment: FilterTest.java
                Filter.java
   

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13438617#comment-13438617 ]

Gilles commented on MATH-851:
-----------------------------

Thanks for your interest.

I'm rather picky about the source code formatting. ;) Please have a look at the discussions that took place in the context of other contributions:
* https://issues.apache.org/jira/browse/MATH-845
* https://issues.apache.org/jira/browse/MATH-815

In addition:
* Local variables must not start with an uppercase letter.
* Be careful not to use bit operators instead of logical ones.
* The exception should probably be {{NoDataException}}.
* In the test class, "10e-14" should be written as "1e-13". Please also refer to the remark about "locality" in test methods, in one of the above references.
* Do not use LaTeX syntax in the Javadoc comments (they should be either ASCII or "readable" HTML).
* The unit tests should also cover the preconditions.

About the design:
I don't think that {{RealVector}} should be used here. The accessor ("getEntry") is likely to be much slower than array access.
Utility (i.e "static") methods are usually collected in a class whose name ends with "...Utils" (that would be "FilterUtils" here) with a unique "private" constructor.
A method name is preferably a verb (i.e. "convolve").
However such choices are to be discussed on the "dev" ML.

               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Updated] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

     [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Clemens Novak updated MATH-851:
-------------------------------

    Attachment: FilterUtilsTest.java
                FilterUtils.java
   

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13438776#comment-13438776 ]

Clemens Novak commented on MATH-851:
------------------------------------

Thanks a lot for your comments and suggestions. I hope I got the issues mentioned above and the source code formatting right. As a side note: I have seen there is a wiki - Would it make sense to collect these general guidelines there? Currently, this information seems to be spread across several tickets... I'd volunteer for that :-)

I assume that there will be a performance loss when using RealVector; on the other hand, RealVector provides many convenience functions a bare metal double[] array lacks (I have a rather strong Matlab/Octave background where everything is vector-based). However, I think this is a architectural decision (a performance comparison between RealVector & double[] might come in handy here)...
               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439277#comment-13439277 ]

Sébastien Brisard commented on MATH-851:
----------------------------------------

Hi Clemens,
thanks for this contribution.

Regarding formatting. The first sentence of a javadoc comment should always end with '.'. This allows the {{javadoc}} tool to extract this first sentence, which is shown in the various summaries. See [How to Write Doc Comments for the Javadoc Tool|http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#firstsentence] -- Side note: although this is not required in CM, I personally try to comply as much as possible with this document: for example, description of arguments should begin with "the", no capital letter, and should not end with '.' ; there are dozens more useless conventions which help having a consistent style. Feel free to *not* adopt them ;) The one I was referring to first *should* be applied.

Regarding the content. I think more details should be given in the Javadoc.
* You should first state that this method does not use the FFT, and should therefore be used only with small kernels (BTW, it would be good to state which is which)
* Also, boundary conditions should be stated explicitely. If I understand correctly, the data {{x[i]}} is zero outside the domain {{i = 0, ..., x.length - 1}}. This is perfectly acceptable, but other conventions could be adopted (linear extrapolation, periodization, ...), so that's something that must be clearly explained.

Finally: do you intend to add other methods to class {{FilterUtils}}. If yes, which ones? Maybe we should think about that before we include this new class.

Thanks for your involvement.

Sébastien

PS: beware the wiki: I've tried to make use of it for several issues. I found it not so convenient, and nearly noone in CM uses it...
               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439392#comment-13439392 ]

Gilles commented on MATH-851:
-----------------------------

bq. [...] although this is not required in CM, I personally try to comply as much as possible with this document: for example, description of arguments should begin with "the", no capital letter, and should not end with '.' [...]

Personally, I don't like this convention because it makes for inconsistent style when there is more than one sentence in the description of the parameter: According to the above, there won't be an uppercase letter to start the first sentencen but there will be one to start the second (according to the rules for writing any document). Also, I do _not_ start the sentence with "the" (with or without uppercase) when the comment is just a short/trivial description (in which case "the" just lengthens the text).
But I _do_ begin the description of the "@return" tag with lowercase "the" (because the generated HTML starts that section with "Returns:").


               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439395#comment-13439395 ]

Sébastien Brisard commented on MATH-851:
----------------------------------------

I knew I should not have written this side note... I was looking for trouble ;)
               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439416#comment-13439416 ]

Gilles commented on MATH-851:
-----------------------------

:)
It's not that I want to impose my preferences, but I find it more pleasant to read code/comments that share some uniform convention. It also makes it easier to spot mistakes.
Obviously, everybody got used to "his" convention and find it more likable than someone else's; however, in a collaborative work, I think that it is worth spelling out the rationale behind one's choice. Then decide which convention should be applied to the Commons Math code.

               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Updated] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

     [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Clemens Novak updated MATH-851:
-------------------------------

    Attachment: FilterUtils.java
                FilterUtilsTest.java
   

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtils.java, FilterUtilsTest.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439565#comment-13439565 ]

Clemens Novak commented on MATH-851:
------------------------------------

I updated the Javadoc (hopefully correct according to your guidelines).

In the meantime I also implemented a convolution based on the FFT (not yet included) and some more related filter functions come to my mind. I assume I should propose my ides on the mailing list for further discussion?
               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtils.java, FilterUtilsTest.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439575#comment-13439575 ]

Sébastien Brisard commented on MATH-851:
----------------------------------------

{quote}
[...] however, in a collaborative work, I think that it is worth spelling out the rationale behind one's choice. Then decide which convention should be applied to the Commons Math code.
{quote}
Your convention actually makes more sense... I have to admit I have not always followed it in the past (not knowing the rationale behind it). I'll do my best in the future!

{quote}
I assume I should propose my ides on the mailing list for further discussion?
{quote}
Yes, that would be the place to go.
               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtils.java, FilterUtilsTest.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439579#comment-13439579 ]

Gilles commented on MATH-851:
-----------------------------

bq. (hopefully correct according to your guidelines).

Unless I'm mistaken, most of the Commons Math code currently follows what I described:
* ~250 occurrences where the "@param" description starts with "the".
* ~5500 occurrences where the "@param" description does not start with "the".

So, unless we decide that "the" is better for some reason (to be stated, and agreed on), I suggest to stick with the de facto consensus: no "the".

               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtils.java, FilterUtilsTest.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439584#comment-13439584 ]

Sébastien Brisard commented on MATH-851:
----------------------------------------

I'm OK with that, only I didn't know there *was* a consensus. I remember having asked the question a few months ago, and being rebuked, the answer being "as long as the comment makes sense, it's OK". That's the reason why I took the liberty to say to Clemens he should feel free to adopt any convention. That was a wrong suggestion and I do apologize.


I'm actually happier with less fuzzy rules, as long as they are stated. From now on, I'll do my best to stick with these rules. Shouldn't we write these rules in the developers guide (I can take care of that)? That would avoid endless discussions?
               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtils.java, FilterUtilsTest.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439613#comment-13439613 ]

Gilles commented on MATH-851:
-----------------------------

(Sadly) There isn't an agreed on convention.
When you asked, when I asked, people said they didn't care much (about consistency).

That's why I wrote *de facto* consensus (i.e. based on the number of occurrences). From which I conclude that people who like consistency should either adopt the most common usage or be ready to correct all the Javadoc according to the a new usage.

               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtils.java, FilterUtilsTest.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Comment Edited] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13439613#comment-13439613 ]

Gilles edited comment on MATH-851 at 8/23/12 2:29 AM:
------------------------------------------------------

(Sadly) There isn't an agreed on convention.
When you asked, when I asked, people said they didn't care much (about consistency).

That's why I wrote *de facto* consensus (i.e. based on the number of occurrences). From which I conclude that people who like consistency should either adopt the most common usage or be ready to correct all the Javadoc according to a new usage.

               
      was (Author: erans):
    (Sadly) There isn't an agreed on convention.
When you asked, when I asked, people said they didn't care much (about consistency).

That's why I wrote *de facto* consensus (i.e. based on the number of occurrences). From which I conclude that people who like consistency should either adopt the most common usage or be ready to correct all the Javadoc according to the a new usage.

                 

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtils.java, FilterUtilsTest.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

       
Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13440185#comment-13440185 ]

Clemens Novak commented on MATH-851:
------------------------------------

Ok - I'm a bit confused by now and hope you can help me better understanding the submission process in this project.

* I write a patch / enhancement to existing project.

* I assume that the two of you (Gilles & Sébastien) have SVN write access and would be able to commit my changes.

* My code needs to follow some guidelines before my changes are accepted. When my code meets these guidelines, then it will be submitted (given that the topic of the enhancement suits the project; something that should have been checked in the beginning via the mailing list. Anyway, guess we are already through this...).


Now this leaves some questions open:

* Who will the potential SVN submitter of the code I have written?

* It seems that there is some dissent about the rules -> Will the guy who will make the submission also "enforce" his rules or do we need to find a consent before any more commits are possible?

In one of the comments above you wrote that you are picky about source code formatting, Javadocs etc... This is perfectly fine with me and I am willing to follow the rules. However, I think we / you need to define the rules somewhere - I looked in the developers manual but this is nowhere as detailed as the comments you gave me here. Above I proposed using the wiki, but you told me that it is not used - Anyway, I don't care where the guidelines are, but the current process of slowly "discovering" the rules one by one doesn't seem to be a very effective way either (with the additional downside that the next contributor has to go through this process as well).

Can we decide on the guidelines and fix them somewhere? This would make contributing for new users much easier... If existing code doesn't follow the rules, it could be cleaned up over time.

Please don't get me wrong - I'm the new guy here, don't know much about procedures and I'm aware that there are some and I need to follow them. However, I'd like to contribute to this library :-)
               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtils.java, FilterUtilsTest.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13440193#comment-13440193 ]

Sébastien Brisard commented on MATH-851:
----------------------------------------

{quote}
Ok - I'm a bit confused by now and hope you can help me better understanding the submission process in this project.
{quote}

That's because I started digressing...

{quote}
* I assume that the two of you (Gilles & Sébastien) have SVN write access and would be able to commit my changes.
{quote}
That's correct.

{quote}
* My code needs to follow some guidelines before my changes are accepted. When my code meets these guidelines, then it will be submitted (given that the topic of the enhancement suits the project; something that should have been checked in the beginning via the mailing list. Anyway, guess we are already through this...).
{quote}
In my view, we are not really _through_ this. I'd rather know what's next, before we start a whole new class {{FilterUtils}}. I think there are real design decisions that should be made before we dash into committing. For example, the boundary conditions I was referring to. Should an enum be defined somewhere, with various boundary conditions? I'm interested in image analysis (same issues with 2D convolution), and various policies are adopted. I'm sure we will find other issues, which need to be discussed. Ideally, we would need some more people involved in signal analysis.

{quote}
Now this leaves some questions open:
{quote}
Yes, I am the culprit :)

{quote}
* Who will the potential SVN submitter of the code I have written?
{quote}
Whoever has some time to do it.

{quote}
* It seems that there is some dissent about the rules -> Will the guy who will make the submission also "enforce" his rules or do we need to find a consent before any more commits are possible?
{quote}
Formatting rules have always (in my view) been a bit fuzzy. It's true you learn by looking at existing code, but since some of the code does not comply with these non-written rules, you sometimes draw wrong conclusions. I personally have been contributing for a year and a half, and still do formatting errors. The discussion above should really not have taken place here, I'm sorry for that. For the time being, I would say: "follow the rules stated by Gilles", and ignore what I have said about formatting (except for the '.' at the end of the first sentence in javadocs).


{quote}
Can we decide on the guidelines and fix them somewhere? This would make contributing for new users much easier... If existing code doesn't follow the rules, it could be cleaned up over time.
{quote}
You are certainly not the first one to ask for this... I will try and write something in the developer's guide.

{quote}
Please don't get me wrong - I'm the new guy here, don't know much about procedures and I'm aware that there are some and I need to follow them. However, I'd like to contribute to this library
{quote}
We do not take you wrong. Your time is precious, and you do not want to spend it reformatting over and over again, just because noone told you how to properly format it in the first place. Thanks a lot for this first contribution, and the contributions to come (Please start a thread on the dev mailing list about these)!

               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtils.java, FilterUtilsTest.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira


Reply | Threaded
Open this post in threaded view
|

[jira] [Commented] (MATH-851) Add convolution

AD_LB (Jira)
In reply to this post by AD_LB (Jira)

    [ https://issues.apache.org/jira/browse/MATH-851?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13440243#comment-13440243 ]

Gilles commented on MATH-851:
-----------------------------

I fully agree with Sébastien: Your proposed code not being committed right away is not (primarily) related to the discussion about formatting rules.
It's rather that accepting a new feature is not automatic; just the fact that it might a useful code snippet  is not enough to include it in a library with some purported design goals (which can be fuzzily defined too, I admit) ;).

One way to speed the process is to demonstrate the usefulness of the new feature in a "real" application. It could be in a non-trivial unit test, or in a new section of the "user guide".

Contributions that already fit within the existing design of Commons Math are also likely to be more rapidly committed. Examples are:
* a bug-fix,
* an efficiency improvement, or
* an implementation of an existing feature (e.g. a new "interpolation" scheme).

Any of those would also help you getting "incrementally" acquainted with the formatting conventions.

               

> Add convolution
> ---------------
>
>                 Key: MATH-851
>                 URL: https://issues.apache.org/jira/browse/MATH-851
>             Project: Commons Math
>          Issue Type: New Feature
>            Reporter: Clemens Novak
>            Priority: Minor
>         Attachments: Filter.java, FilterTest.java, FilterUtils.java, FilterUtils.java, FilterUtilsTest.java, FilterUtilsTest.java
>
>
> I created a function performing (one-dimensional) convolution. Currently, the function is in a class called Filter, but I am not sure whether this is the appropriate place for it(?).

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira