Hello. [Side question: Do you send a copy of your posts directly to me? If so, that is not necessary and is even annoying because when I hit "reply" in my mail client, the conversation continues off-list...]
Le mar. 25 févr. 2020 à 14:53, CT <chentao...@qq.com> a écrit : > > Hi Gilles: > Sorry for my unfamiliar in contribution. I started a new PR for most of > your suggestion: > https://github.com/apache/commons-math/pull/120 Sorry for seemingly nit-picking but the global issue is the same as with PR #118: It contains too many unrelated changes. There should be *one* PR for each batch of significant changes. More precisely, for this work, that would entail (a.o. things), one PR for each of the following: * Class "ClusterUtils" (design yet to be discussed) * Factoring out whatever code is necessary for your proposed feature (that would otherwise be duplicated) We try and avoid bloating the API, hence the changes which I've suggested: * remove the constructors with default parameters Overall, each PR should probably contain a single commit, in order to ease review. > I remain have one question below: > > ------------------ Original ------------------ > From: "Gilles Sadowski"<gillese...@gmail.com>; > Date: Mon, Feb 24, 2020 09:52 PM > To: "dev"<dev@commons.apache.org>; > Subject: Re: [math] Discuss: New feature MiniBatchKMeansClusterer > > >Hi. > > > >Le sam. 22 févr. 2020 à 14:37, CT <chentao...@qq.com> a écrit : > >> > >> Hi Gilles: > >> I really appricate for your patient to help me to solve the mail sending > >> problem, I try to set the only setting about charset "Use >Unicode" for > >> this mail. > > > >The contents seems fine. However, there is something wrong: replying to your > >message sends to you instead of the mailing list... > > > >> I have created a pull request: > >> https://github.com/apache/commons-math/pull/118 > > > >That PR fails the build: > > > > https://travis-ci.org/apache/commons-math/builds/653293451?utm_source=github_status&utm_medium=notification > > > >Also, the code doesn't take into account all the remarks which I > >made in previous comments (e.g. file JIRA reports to allow tracking > >of changes to existing code and not mix those with new code). > > > > I did not get what kinds of problem should tracking by JIRA. > In my opinion all the change is related to my PR. Yes, they are related but they could be provided in more focused PRs as explained above. > The JIRA issue about Feature MiniBatch has been closed. It is not closed: https://issues.apache.org/jira/projects/MATH/issues/MATH-1509 > Should I fire a new issue? You should create one JIRA issue per self-contained change. You could also create "sub-tasks" of MATH-1509 (for changes that are strongly related to your contribution). > > >In addition, you should avoid mixing massive cosmetic changes (e.g. > >Javadoc reformatting) within a commit than contains other types of > >change: > > > > https://github.com/apache/commons-math/pull/118/commits/47a055e6264d084547854f9290461f020f2131cf > > > >> And a comparsion between KMeans and MiniBatchKMeans by using the > >> >"org.apache.commons.math4.userguide.ClusterAlgorithmComparison": > > > >Functionality seems to work fine, but ability to review incremental > >changes is extremely important in a collaborative project. > > > > Thanks for your remind, and I created a new PR that improve the problems one > by one. I hope that I've now fixed the misunderstanding, Gilles > > [...] --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
