You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Gilles Sadowski <gi...@gmail.com> on 2020/06/06 11:32:56 UTC
[Math] PR #143
Hi.
2020-06-06 10:18 UTC+02:00, GitBox <gi...@apache.org>:
>
> XenoAmess opened a new pull request #143:
> URL: https://github.com/apache/commons-math/pull/143
>
>
> use System.arraycopy instead of loop.
>
There are several issues with this PR.
It contains several commits, among which some seem,
IIUC, to partly revert a previous change in that same PR.
It that situation, you must squash the commits (so that
the net changes stand out).
Also, the commit messages are uninformative ("refine",
"fix false positive").
The PR contains different types of changes: one is "use
arraycopy" but the others are not documented anywhere
(commit message, JIRA).
Traceability and easy reviewing are requirements for
"Commons" components.
For example, modifying the code that fills an array is
not a "minor" change, even if just because it makes the
reviewer wonder "Why?" (and the answer is nowhere to
be found from the official tracking tools).
If some performance improvement is unexpected from
looking at the code change, benchmark are indeed
necessary especially if the new code is less clear (and
a summary of the results should certainly make it into
the commit message and/or the corresponding JIRA
report). [Also, an appropriately named utility method
is probably better than inline statements.]
Regards,
Gilles
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] PR #143
Posted by Gilles Sadowski <gi...@gmail.com>.
Hello.
2020-06-06 13:47 UTC+02:00, Xeno Amess <xe...@gmail.com>:
> Yes you are right.
>>you must squash the commits.
>>the commit messages are uninformative
>
> Usually I'd do them when commiter think this pr is ok,
As said before, it's needlessly difficult to figure out the
net changes when they result from several commits.
> and then tell me we
> should merge it, and at that moment I squash it, and it get merged.
> the commit messages will be fixed at that time too.
The commit message should not need any fixing after
the commit. It must correspond to what was done, and
would often help the reviewer. [Why make it more
complicated for us to accept the PR?]
>>partly revert a previous change
>
> Yes, as my newest performance test shows something is not quite right in
> my previous changes. It makes the codes even slower.
> So I changed that part.
But this back-and-forth is not something interesting to
keep in the history if we can avoid it.
>
>>benchmark are indeed necessary
>
> agreed, I did a benchmark yesterday, and pointed a link on github pages.
> I'll copy/paste
>
>>The PR contains different types of changes: one is "use arraycopy" but the
> others are not documented anywhere (commit message, JIRA).
> Ah, I forgot to create a jira ticket for it. sorry about that.
>
> So, in conclusion, I will close this pr, and make two jira tickets, for the
> two different types of refines.
Thanks,
Gilles
> > [...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] PR #143
Posted by Gilles Sadowski <gi...@gmail.com>.
Hi.
Le dim. 7 juin 2020 à 15:07, Gilles Sadowski <gi...@gmail.com> a écrit :
>
> [...]
>
> The current flood of email messages from GitHub is not sustainable
> for me (sorry).
I'm not registered on GitHub; hence even when I agree with your
proposed changes (e.g. PR #152), I cannot comment directly over
there.
In that instance, the commit is fine but the message should just be
akin to "Follow best practice to test ordering in implementation of
Comparable".
Once we know the general "category", the rest of your message is
just repeating what can be readily deduced from the diff. [The full
explanation is quite welcome, but for such a one-liner, it is enough
that it appears on the JIRA page.]
Best regards,
Gilles
> > > >> > [...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] PR #143
Posted by Gilles Sadowski <gi...@gmail.com>.
Hi.
Le dim. 7 juin 2020 à 12:54, Xeno Amess <xe...@gmail.com> a écrit :
>
> Ahhh you are right.
> I'm new to JIRA. sorrrrry
I'll take the opportunity to draw your attention to pending issues in
"Commons Math"[1] that would prevent releasing the next version.
It's great that you want to clean up the codebase, but the myriad of
little adjustments (among the few significant enhancements) will not
make the release happen sooner.
I hope that you can find something interesting to work on from the
list below (to which you can add of course by filing new JIRA reports).
As for the clean-up, I strongly suggest to create *one* report that will
list all suggestions/PRs for small improvements sorted in categories
(with *one* JIRA "sub-task" per category).
That way, a reviewer can take on a single type of issue, quickly
figuring out what the change is about, in every commit. A single
"target" issue on JIRA would also allow to normalize/simplify/improve
the commit message. For example, a message such as this one:
---CUT---
refine equals from ClassCastException
---CUT---
is not to the point and one is clueless without actually looking at the
diff.
This kind of issue should rather lead to a "Do not use exception for
control flow" category. [Then the commit message would be just that
(prefixed with the JIRA issue identifier).]
The current flood of email messages from GitHub is not sustainable
for me (sorry).
Thanks,
Gilles
[1] https://issues.apache.org/jira/projects/MATH/issues
> Gilles Sadowski <gi...@gmail.com> 于2020年6月7日周日 下午6:50写道：
>
> > Hi.
> >
> > 2020-06-07 8:21 UTC+02:00, Xeno Amess <xe...@gmail.com>:
> > > If you are telling about 144:
> > > yes, original codes runs like:
> > > f[0] = f0;
> > > f[1] = f1;
> > > f[2] = f[2-2] = f[0] = f[0]
> > > f[3] = f[3-2] = f[1] = f[1]
> > > f[4] = f[4-2] = f[2] = f[0]
> > > f[5] = f[5-2] = f[3] = f[1]
> > >
> > > the new codes runs like:
> > > f[0] = f0;
> > > f[1] = f1;
> > > f[2] = 2&1==0 ? f0 : f1 = f0
> > > f[3] = 3&1==0 ? f0 : f1 = f1
> > > f[4] = 4&1==0 ? f0 : f1 = f0
> > > f[5] = 5&1==0 ? f0 : f1 = f1
> > >
> > > thus it runs faster for around 7% faster on my performance tests.
> > >
> >
> > Wrong thread.
> > Please reply on JIRA, where this discussion should be taking place.[1]
> >
> > Regards,
> > Gilles
> >
> > [1] https://issues.apache.org/jira/browse/MATH-1538
> >
> > >
> > >
> > > Gilles Sadowski <gi...@gmail.com> 于2020年6月7日周日 上午5:53写道：
> > >
> > >> Hi.
> > >>
> > >> 2020-06-06 14:21 UTC+02:00, Xeno Amess <xe...@gmail.com>:
> > >> > Alright, done.
> > >> > This pr is now splitted into two prs.
> > >> > https://github.com/apache/commons-math/pull/144
> > >> > https://github.com/apache/commons-math/pull/143
> > >>
> > >> I think that there are issues with some of the changes (e.g. where
> > >> the previous code used an "index" variable but in the new version
> > >> there is a hard-coded number).
> > >>
> > >> Gilles
> > >>
> > >> > [...]
> > >>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] PR #143
Posted by Xeno Amess <xe...@gmail.com>.
Ahhh you are right.
I'm new to JIRA. sorrrrry
Gilles Sadowski <gi...@gmail.com> 于2020年6月7日周日 下午6:50写道：
> Hi.
>
> 2020-06-07 8:21 UTC+02:00, Xeno Amess <xe...@gmail.com>:
> > If you are telling about 144:
> > yes, original codes runs like:
> > f[0] = f0;
> > f[1] = f1;
> > f[2] = f[2-2] = f[0] = f[0]
> > f[3] = f[3-2] = f[1] = f[1]
> > f[4] = f[4-2] = f[2] = f[0]
> > f[5] = f[5-2] = f[3] = f[1]
> >
> > the new codes runs like:
> > f[0] = f0;
> > f[1] = f1;
> > f[2] = 2&1==0 ? f0 : f1 = f0
> > f[3] = 3&1==0 ? f0 : f1 = f1
> > f[4] = 4&1==0 ? f0 : f1 = f0
> > f[5] = 5&1==0 ? f0 : f1 = f1
> >
> > thus it runs faster for around 7% faster on my performance tests.
> >
>
> Wrong thread.
> Please reply on JIRA, where this discussion should be taking place.[1]
>
> Regards,
> Gilles
>
> [1] https://issues.apache.org/jira/browse/MATH-1538
>
> >
> >
> > Gilles Sadowski <gi...@gmail.com> 于2020年6月7日周日 上午5:53写道：
> >
> >> Hi.
> >>
> >> 2020-06-06 14:21 UTC+02:00, Xeno Amess <xe...@gmail.com>:
> >> > Alright, done.
> >> > This pr is now splitted into two prs.
> >> > https://github.com/apache/commons-math/pull/144
> >> > https://github.com/apache/commons-math/pull/143
> >>
> >> I think that there are issues with some of the changes (e.g. where
> >> the previous code used an "index" variable but in the new version
> >> there is a hard-coded number).
> >>
> >> Gilles
> >>
> >> > [...]
> >>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [Math] PR #143
Posted by Gilles Sadowski <gi...@gmail.com>.
Hi.
2020-06-07 8:21 UTC+02:00, Xeno Amess <xe...@gmail.com>:
> If you are telling about 144:
> yes, original codes runs like:
> f[0] = f0;
> f[1] = f1;
> f[2] = f[2-2] = f[0] = f[0]
> f[3] = f[3-2] = f[1] = f[1]
> f[4] = f[4-2] = f[2] = f[0]
> f[5] = f[5-2] = f[3] = f[1]
>
> the new codes runs like:
> f[0] = f0;
> f[1] = f1;
> f[2] = 2&1==0 ? f0 : f1 = f0
> f[3] = 3&1==0 ? f0 : f1 = f1
> f[4] = 4&1==0 ? f0 : f1 = f0
> f[5] = 5&1==0 ? f0 : f1 = f1
>
> thus it runs faster for around 7% faster on my performance tests.
>
Wrong thread.
Please reply on JIRA, where this discussion should be taking place.[1]
Regards,
Gilles
[1] https://issues.apache.org/jira/browse/MATH-1538
>
>
> Gilles Sadowski <gi...@gmail.com> 于2020年6月7日周日 上午5:53写道：
>
>> Hi.
>>
>> 2020-06-06 14:21 UTC+02:00, Xeno Amess <xe...@gmail.com>:
>> > Alright, done.
>> > This pr is now splitted into two prs.
>> > https://github.com/apache/commons-math/pull/144
>> > https://github.com/apache/commons-math/pull/143
>>
>> I think that there are issues with some of the changes (e.g. where
>> the previous code used an "index" variable but in the new version
>> there is a hard-coded number).
>>
>> Gilles
>>
>> > [...]
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] PR #143
Posted by Xeno Amess <xe...@gmail.com>.
If you are telling about 144:
yes, original codes runs like:
f[0] = f0;
f[1] = f1;
f[2] = f[2-2] = f[0] = f[0]
f[3] = f[3-2] = f[1] = f[1]
f[4] = f[4-2] = f[2] = f[0]
f[5] = f[5-2] = f[3] = f[1]
the new codes runs like:
f[0] = f0;
f[1] = f1;
f[2] = 2&1==0 ? f0 : f1 = f0
f[3] = 3&1==0 ? f0 : f1 = f1
f[4] = 4&1==0 ? f0 : f1 = f0
f[5] = 5&1==0 ? f0 : f1 = f1
thus it runs faster for around 7% faster on my performance tests.
Gilles Sadowski <gi...@gmail.com> 于2020年6月7日周日 上午5:53写道：
> Hi.
>
> 2020-06-06 14:21 UTC+02:00, Xeno Amess <xe...@gmail.com>:
> > Alright, done.
> > This pr is now splitted into two prs.
> > https://github.com/apache/commons-math/pull/144
> > https://github.com/apache/commons-math/pull/143
>
> I think that there are issues with some of the changes (e.g. where
> the previous code used an "index" variable but in the new version
> there is a hard-coded number).
>
> Gilles
>
> > [...]
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: [Math] PR #143
Posted by Gilles Sadowski <gi...@gmail.com>.
Hi.
2020-06-06 14:21 UTC+02:00, Xeno Amess <xe...@gmail.com>:
> Alright, done.
> This pr is now splitted into two prs.
> https://github.com/apache/commons-math/pull/144
> https://github.com/apache/commons-math/pull/143
I think that there are issues with some of the changes (e.g. where
the previous code used an "index" variable but in the new version
there is a hard-coded number).
Gilles
> [...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [Math] PR #143
Posted by Xeno Amess <xe...@gmail.com>.
Alright, done.
This pr is now splitted into two prs.
https://github.com/apache/commons-math/pull/144
https://github.com/apache/commons-math/pull/143
Thx.
Xeno Amess <xe...@gmail.com> 于2020年6月6日周六 下午7:47写道：
> Yes you are right.
> >you must squash the commits.
> >the commit messages are uninformative
>
> Usually I'd do them when commiter think this pr is ok, and then tell me we
> should merge it, and at that moment I squash it, and it get merged.
> the commit messages will be fixed at that time too.
>
> >partly revert a previous change
>
> Yes, as my newest performance test shows something is not quite right in
> my previous changes. It makes the codes even slower.
> So I changed that part.
>
> >benchmark are indeed necessary
>
> agreed, I did a benchmark yesterday, and pointed a link on github pages.
> I'll copy/paste
>
> >The PR contains different types of changes: one is "use arraycopy" but
> the others are not documented anywhere (commit message, JIRA).
> Ah, I forgot to create a jira ticket for it. sorry about that.
>
> So, in conclusion, I will close this pr, and make two jira tickets, for
> the two different types of refines.
>
> Thanks for your help.
>
> Gilles Sadowski <gi...@gmail.com> 于2020年6月6日周六 下午7:33写道：
>
>> Hi.
>>
>> 2020-06-06 10:18 UTC+02:00, GitBox <gi...@apache.org>:
>> >
>> > XenoAmess opened a new pull request #143:
>> > URL: https://github.com/apache/commons-math/pull/143
>> >
>> >
>> > use System.arraycopy instead of loop.
>> >
>>
>> There are several issues with this PR.
>>
>> It contains several commits, among which some seem,
>> IIUC, to partly revert a previous change in that same PR.
>> It that situation, you must squash the commits (so that
>> the net changes stand out).
>> Also, the commit messages are uninformative ("refine",
>> "fix false positive").
>> The PR contains different types of changes: one is "use
>> arraycopy" but the others are not documented anywhere
>> (commit message, JIRA).
>> Traceability and easy reviewing are requirements for
>> "Commons" components.
>> For example, modifying the code that fills an array is
>> not a "minor" change, even if just because it makes the
>> reviewer wonder "Why?" (and the answer is nowhere to
>> be found from the official tracking tools).
>> If some performance improvement is unexpected from
>> looking at the code change, benchmark are indeed
>> necessary especially if the new code is less clear (and
>> a summary of the results should certainly make it into
>> the commit message and/or the corresponding JIRA
>> report). [Also, an appropriately named utility method
>> is probably better than inline statements.]
>>
>> Regards,
>> Gilles
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
Re: [Math] PR #143
Posted by Xeno Amess <xe...@gmail.com>.
Yes you are right.
>you must squash the commits.
>the commit messages are uninformative
Usually I'd do them when commiter think this pr is ok, and then tell me we
should merge it, and at that moment I squash it, and it get merged.
the commit messages will be fixed at that time too.
>partly revert a previous change
Yes, as my newest performance test shows something is not quite right in
my previous changes. It makes the codes even slower.
So I changed that part.
>benchmark are indeed necessary
agreed, I did a benchmark yesterday, and pointed a link on github pages.
I'll copy/paste
>The PR contains different types of changes: one is "use arraycopy" but the
others are not documented anywhere (commit message, JIRA).
Ah, I forgot to create a jira ticket for it. sorry about that.
So, in conclusion, I will close this pr, and make two jira tickets, for the
two different types of refines.
Thanks for your help.
Gilles Sadowski <gi...@gmail.com> 于2020年6月6日周六 下午7:33写道：
> Hi.
>
> 2020-06-06 10:18 UTC+02:00, GitBox <gi...@apache.org>:
> >
> > XenoAmess opened a new pull request #143:
> > URL: https://github.com/apache/commons-math/pull/143
> >
> >
> > use System.arraycopy instead of loop.
> >
>
> There are several issues with this PR.
>
> It contains several commits, among which some seem,
> IIUC, to partly revert a previous change in that same PR.
> It that situation, you must squash the commits (so that
> the net changes stand out).
> Also, the commit messages are uninformative ("refine",
> "fix false positive").
> The PR contains different types of changes: one is "use
> arraycopy" but the others are not documented anywhere
> (commit message, JIRA).
> Traceability and easy reviewing are requirements for
> "Commons" components.
> For example, modifying the code that fills an array is
> not a "minor" change, even if just because it makes the
> reviewer wonder "Why?" (and the answer is nowhere to
> be found from the official tracking tools).
> If some performance improvement is unexpected from
> looking at the code change, benchmark are indeed
> necessary especially if the new code is less clear (and
> a summary of the results should certainly make it into
> the commit message and/or the corresponding JIRA
> report). [Also, an appropriately named utility method
> is probably better than inline statements.]
>
> Regards,
> Gilles
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>