You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Eric Barnhill <er...@gmail.com> on 2015/10/28 17:33:54 UTC
[MATH] ComplexUtils pull request; some future proposals
Dear all,
Thanks for the feedback on ComplexUtils and I have submitted a pull request
with the edits.
I have added functionality to convert between Complex arrays and real
double, real float, interleaved double, interleaved float, split double
(one real, one imag) and split float arrays. Per Gilles' suggestion I have
included the use of IntegerSequence.range() arguments and the methods
mostly refer to workhorse Extract() methods. I also added a method to
initialize Complex[] arrays to all zeros, to avoid null pointer exceptions.
I have also updated ComplexUtilsTest to include JUnit tests of every
method, which all pass. I had to define many arrays to run the tests and
hopefully the comments make the purpose of each clear. I was not entirely
clear how to use the tolerance parameter and submitted ulp each time, so
contact me if I should do something else.
I am using nearly all of these methods in the medical imaging library I am
working on so I don't think I overdid it.
Repeating the methods for different types, so many times, got me to
thinking about the overall structure of arrays in math functions and what I
would most like to see. So in my own library I am creating a MathArray
superclass. This in turn can be Complex, real, split, or interleaved, and
contain any type of primitive, kind of like the ImageJ ImageProcessor
interface This will allow every method I have put in ComplexUtils, and many
more I keep in other libraries, to be written only once for the superclass.
Further the superclass will have a full range of math methods that operate
on arrays element wise, enabling syntax approaching Matlab to be used for
arrays that is basically the same as what is already in place for Complex
objects, i.e. commands like
array1.multiply(array2).pwr(2).zeroPad(n).resizeBicubic(2).stripPadding(n/2)
.
If this is of interest to the community I can submit a pull request when
I've got it worked up. If someone has already designed such a library I'd
be grateful if someone mentions it before I get started.
Best,
Eric
Re: [MATH] ComplexUtils pull request; some future proposals
Posted by Nick Burch <ni...@apache.org>.
On Fri, 30 Oct 2015, luc wrote:
> In fact, pull requests are really github-specific, and we don't have a
> complete integration between our Apache infrastructure and github for
> Commons Math. As an example, the requests are not directly forwarded to
> our mailing lists so we may miss them (and we already missed several
> ones ...).
You just need to ask infra to turn on notifications!
If you let them know the github mirror, and what list to have
notifications sent to, the infrabot will post to the list every time a
pull request is created or updated. It'll also allow you to have the pull
requests auto-closed with certain commit messages
Once enabled, you start receiving emails like this for pull requests:
http://mail-archives.apache.org/mod_mbox/poi-dev/201510.mbox/%3Cgit-pr-24-poi%40git.apache.org%3E
Nick
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [MATH] ComplexUtils pull request; some future proposals
Posted by luc <lu...@spaceroots.org>.
Le 2015-10-30 10:58, Gilles a écrit :
> On Fri, 30 Oct 2015 09:20:00 +0000, Eric Barnhill wrote:
>> On 30/10/15 02:15, Gilles wrote:
>>>
>>> There are some problems with the Javadoc (wrong "@return" comment).
>>> Not all local variables that are constant are declared "final".
>>
>> I am happy to give it all another proof read. I take it the procedure
>> is to fork the dedicated branch and then submit a pull request to that
>> branch?
>
> I couldn't say; we don't have a procedure yet... :-}
In fact, pull requests are really github-specific, and we don't have
a complete integration between our Apache infrastructure and github
for Commons Math. As an example, the requests are not directly
forwarded to our mailing lists so we may miss them (and we already
missed several ones ...).
One Apache motto is: "if it didn't happen on the mailing lists,
it didn't happen". So the best medium for discussion is this mailing
list. The best medium for registering issues, evolution proposals
and patches is our JIRA instance. Typically, issues start at JIRA,
are discussed here if the discussion is lengthy or need to get
public oversight, and the patches coming for non-committers are attached
to the JIRA issue (our mailing list strips attachments).
best regards,
Luc
>
>>>
>>> Shouldn't independent changes be performed in separate commits?
>>> [Referring to "IntegerSequence" and "LaguerreSolver".]
>>
>> I made edits to the Solver in particular because my commit would
>> otherwise have left it broken.
>
> Since you added new methods, that should not be the case.
> Whenever possible, it's clearer to modify calls in other parts of the
> library in a separate commit, and then remove obsolete methods in a
> third step.
>
>> If that is not best practice I am happy
>> to revert and submit independently.
>>
>>>
>>> Actually, I don't like the new "size" method in "IntegerSequence".
>>> Although the number of elements is needed in the new methods in
>>> ComplexUtils, I don't think that we should advertise the
>>> functionality
>>> in its current (necessarily) poor implementation.
>>> A better alternative is to have an instance that will hold the
>>> number of elements it contains:
>>> https://issues.apache.org/jira/browse/MATH-1286
>>
>> My first impulse was to see if I could add a field to the range
>> object that contained its size. But as the method returns an Iterator
>> I didn't see a way to do that without returning a subclass that
>> extended Iterator in some way instead. I figured that was not a
>> desired outcome. So instead I incorporated what as far as I know is
>> best practice:
>>
>>
>> http://stackoverflow.com/questions/9720195/what-is-the-best-way-to-get-the-count-length-size-of-an-iterator
>
> They don't really say it's best practice, rather, it's all you can
> do with an Iterator. ;-)
>
>> Alternatively I could just create a local private method in
>> ComplexUtils that determined the size by iterating through the range
>> and call that, and leave IntegerSequence alone.
>
> When MATH-1286 is resolved, that won't be necessary.
>
> Best regards,
> Gilles
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [MATH] ComplexUtils pull request; some future proposals
Posted by Gilles <gi...@harfang.homelinux.org>.
On Fri, 30 Oct 2015 09:20:00 +0000, Eric Barnhill wrote:
> On 30/10/15 02:15, Gilles wrote:
>>
>> There are some problems with the Javadoc (wrong "@return" comment).
>> Not all local variables that are constant are declared "final".
>
> I am happy to give it all another proof read. I take it the procedure
> is to fork the dedicated branch and then submit a pull request to
> that
> branch?
I couldn't say; we don't have a procedure yet... :-}
>>
>> Shouldn't independent changes be performed in separate commits?
>> [Referring to "IntegerSequence" and "LaguerreSolver".]
>
> I made edits to the Solver in particular because my commit would
> otherwise have left it broken.
Since you added new methods, that should not be the case.
Whenever possible, it's clearer to modify calls in other parts of the
library in a separate commit, and then remove obsolete methods in a
third step.
> If that is not best practice I am happy
> to revert and submit independently.
>
>>
>> Actually, I don't like the new "size" method in "IntegerSequence".
>> Although the number of elements is needed in the new methods in
>> ComplexUtils, I don't think that we should advertise the
>> functionality
>> in its current (necessarily) poor implementation.
>> A better alternative is to have an instance that will hold the
>> number of elements it contains:
>> https://issues.apache.org/jira/browse/MATH-1286
>
> My first impulse was to see if I could add a field to the range
> object that contained its size. But as the method returns an Iterator
> I didn't see a way to do that without returning a subclass that
> extended Iterator in some way instead. I figured that was not a
> desired outcome. So instead I incorporated what as far as I know is
> best practice:
>
>
> http://stackoverflow.com/questions/9720195/what-is-the-best-way-to-get-the-count-length-size-of-an-iterator
They don't really say it's best practice, rather, it's all you can
do with an Iterator. ;-)
> Alternatively I could just create a local private method in
> ComplexUtils that determined the size by iterating through the range
> and call that, and leave IntegerSequence alone.
When MATH-1286 is resolved, that won't be necessary.
Best regards,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [MATH] ComplexUtils pull request; some future proposals
Posted by Eric Barnhill <er...@gmail.com>.
On 30/10/15 02:15, Gilles wrote:
>
> There are some problems with the Javadoc (wrong "@return" comment).
> Not all local variables that are constant are declared "final".
I am happy to give it all another proof read. I take it the procedure is
to fork the dedicated branch and then submit a pull request to that branch?
>
> Shouldn't independent changes be performed in separate commits?
> [Referring to "IntegerSequence" and "LaguerreSolver".]
I made edits to the Solver in particular because my commit would
otherwise have left it broken. If that is not best practice I am happy
to revert and submit independently.
>
> Actually, I don't like the new "size" method in "IntegerSequence".
> Although the number of elements is needed in the new methods in
> ComplexUtils, I don't think that we should advertise the functionality
> in its current (necessarily) poor implementation.
> A better alternative is to have an instance that will hold the
> number of elements it contains:
> https://issues.apache.org/jira/browse/MATH-1286
My first impulse was to see if I could add a field to the range object
that contained its size. But as the method returns an Iterator I didn't
see a way to do that without returning a subclass that extended Iterator
in some way instead. I figured that was not a desired outcome. So
instead I incorporated what as far as I know is best practice:
http://stackoverflow.com/questions/9720195/what-is-the-best-way-to-get-the-count-length-size-of-an-iterator
Alternatively I could just create a local private method in ComplexUtils
that determined the size by iterating through the range and call that,
and leave IntegerSequence alone.
Best,
Eric
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [MATH] ComplexUtils pull request; some future proposals
Posted by Gilles <gi...@harfang.homelinux.org>.
Hello.
On Thu, 29 Oct 2015 20:24:56 +0100, Luc Maisonobe wrote:
> Le 28/10/2015 17:33, Eric Barnhill a écrit :
>> Dear all,
>
> Hi Eric,
>
>>
>> Thanks for the feedback on ComplexUtils and I have submitted a pull
>> request
>> with the edits.
>
> I have pulled this request into our git repository, with minor edits.
> The changes are mainly removing tabs and trailing blanks, and fixing
> some javadocs.
>
> In the initialize methods, I have also replaced the construction of
> a bunch of new Complex(0, 0) with the reuse of the constant
> Complex.ZERO. As our complex instances are immutable, it seemed
> better
> to me this way.
>
> For now, it is in a dedicated branch named
> complex-and-primitive-arrays,
> so the pull request may not close automatically as it is not on
> master.
>
> Do other developers agree with merging this branch to master?
There are some problems with the Javadoc (wrong "@return" comment).
Not all local variables that are constant are declared "final".
Shouldn't independent changes be performed in separate commits?
[Referring to "IntegerSequence" and "LaguerreSolver".]
Actually, I don't like the new "size" method in "IntegerSequence".
Although the number of elements is needed in the new methods in
ComplexUtils, I don't think that we should advertise the functionality
in its current (necessarily) poor implementation.
A better alternative is to have an instance that will hold the
number of elements it contains:
https://issues.apache.org/jira/browse/MATH-1286
Best regards,
Gilles
>> [...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [MATH] ComplexUtils pull request; some future proposals
Posted by Gilles <gi...@harfang.homelinux.org>.
On Fri, 30 Oct 2015 09:25:07 +0000, Eric Barnhill wrote:
> On 30/10/15 02:35, Gilles wrote:
>>
>> Unless I'm mistaken, there is no report on the project's
>> bug-tracking system:
>> https://issues.apache.org/jira/browse/MATH
>>
>> Could you please open one?
>
> I see that I need someone to assign me the create issue project
> permission. Can I request that here?
You should be able to create issues; if that's not the case, could you
please post the error you encounter?
Thanks,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [MATH] ComplexUtils pull request; some future proposals
Posted by Eric Barnhill <er...@gmail.com>.
On 30/10/15 02:35, Gilles wrote:
>
> Unless I'm mistaken, there is no report on the project's bug-tracking
> system:
> https://issues.apache.org/jira/browse/MATH
>
> Could you please open one?
I see that I need someone to assign me the create issue project
permission. Can I request that here?
Eric
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [MATH] ComplexUtils pull request; some future proposals
Posted by Gilles <gi...@harfang.homelinux.org>.
On Thu, 29 Oct 2015 20:24:56 +0100, Luc Maisonobe wrote:
> Le 28/10/2015 17:33, Eric Barnhill a écrit :
>> Dear all,
>
> Hi Eric,
>
>>
>> Thanks for the feedback on ComplexUtils and I have submitted a pull
>> request
>> with the edits.
Unless I'm mistaken, there is no report on the project's bug-tracking
system:
https://issues.apache.org/jira/browse/MATH
Could you please open one?
Thanks,
Gilles
>> [...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [MATH] ComplexUtils pull request; some future proposals
Posted by Luc Maisonobe <lu...@spaceroots.org>.
Le 28/10/2015 17:33, Eric Barnhill a écrit :
> Dear all,
Hi Eric,
>
> Thanks for the feedback on ComplexUtils and I have submitted a pull request
> with the edits.
I have pulled this request into our git repository, with minor edits.
The changes are mainly removing tabs and trailing blanks, and fixing
some javadocs.
In the initialize methods, I have also replaced the construction of
a bunch of new Complex(0, 0) with the reuse of the constant
Complex.ZERO. As our complex instances are immutable, it seemed better
to me this way.
For now, it is in a dedicated branch named complex-and-primitive-arrays,
so the pull request may not close automatically as it is not on master.
Do other developers agree with merging this branch to master?
>
> I have added functionality to convert between Complex arrays and real
> double, real float, interleaved double, interleaved float, split double
> (one real, one imag) and split float arrays. Per Gilles' suggestion I have
> included the use of IntegerSequence.range() arguments and the methods
> mostly refer to workhorse Extract() methods. I also added a method to
> initialize Complex[] arrays to all zeros, to avoid null pointer exceptions.
>
> I have also updated ComplexUtilsTest to include JUnit tests of every
> method, which all pass. I had to define many arrays to run the tests and
> hopefully the comments make the purpose of each clear. I was not entirely
> clear how to use the tolerance parameter and submitted ulp each time, so
> contact me if I should do something else.
>
> I am using nearly all of these methods in the medical imaging library I am
> working on so I don't think I overdid it.
>
> Repeating the methods for different types, so many times, got me to
> thinking about the overall structure of arrays in math functions and what I
> would most like to see. So in my own library I am creating a MathArray
> superclass. This in turn can be Complex, real, split, or interleaved, and
> contain any type of primitive, kind of like the ImageJ ImageProcessor
> interface This will allow every method I have put in ComplexUtils, and many
> more I keep in other libraries, to be written only once for the superclass.
> Further the superclass will have a full range of math methods that operate
> on arrays element wise, enabling syntax approaching Matlab to be used for
> arrays that is basically the same as what is already in place for Complex
> objects, i.e. commands like
> array1.multiply(array2).pwr(2).zeroPad(n).resizeBicubic(2).stripPadding(n/2)
> .
>
> If this is of interest to the community I can submit a pull request when
> I've got it worked up. If someone has already designed such a library I'd
> be grateful if someone mentions it before I get started.
This should be discussed in a separate thread.
best regards,
Luc
>
> Best,
> Eric
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org