Re: [Math] PR #143
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 于2020年6月6日周六 下午7:33写道: > Hi. > > 2020-06-06 10:18 UTC+02:00, GitBox : > > > > 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-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
Re: [Math] PR #143
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 于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 于2020年6月6日周六 下午7:33写道: > >> Hi. >> >> 2020-06-06 10:18 UTC+02:00, GitBox : >> > >> > 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-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >>
Re: [Math] PR #143
Hello. 2020-06-06 13:47 UTC+02:00, Xeno Amess : > 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-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [Math] PR #143
Hi. 2020-06-06 14:21 UTC+02:00, Xeno Amess : > 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-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [Math] PR #143
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 于2020年6月7日周日 上午5:53写道: > Hi. > > 2020-06-06 14:21 UTC+02:00, Xeno Amess : > > 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-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
Re: [Math] PR #143
Hi. 2020-06-07 8:21 UTC+02:00, Xeno Amess : > 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 于2020年6月7日周日 上午5:53写道: > >> Hi. >> >> 2020-06-06 14:21 UTC+02:00, Xeno Amess : >> > 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-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [Math] PR #143
Ahhh you are right. I'm new to JIRA. sory Gilles Sadowski 于2020年6月7日周日 下午6:50写道: > Hi. > > 2020-06-07 8:21 UTC+02:00, Xeno Amess : > > 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 于2020年6月7日周日 上午5:53写道: > > > >> Hi. > >> > >> 2020-06-06 14:21 UTC+02:00, Xeno Amess : > >> > 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-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
Re: [Math] PR #143
Hi. Le dim. 7 juin 2020 à 12:54, Xeno Amess a écrit : > > Ahhh you are right. > I'm new to JIRA. sory 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 于2020年6月7日周日 下午6:50写道: > > > Hi. > > > > 2020-06-07 8:21 UTC+02:00, Xeno Amess : > > > 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 于2020年6月7日周日 上午5:53写道: > > > > > >> Hi. > > >> > > >> 2020-06-06 14:21 UTC+02:00, Xeno Amess : > > >> > 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-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [Math] PR #143
Hi. Le dim. 7 juin 2020 à 15:07, Gilles Sadowski 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-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org