Hi Gilles,
------------------ Original ------------------ From: "GillesSadowski"<gillese...@gmail.com>; Date: Wed, Feb 26, 2020 00:32 AM To: "Commons Developers List"<dev@commons.apache.org>; Subject: Re: [math] Discuss: New feature MiniBatchKMeansClusterer >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...] Yes, I always copy to you, I found the ML never forward my email up to now (that I subscribe the ML with my other email and never received my own email). Sorry for annoying you, but you are the only person received my email because I CC to you. >>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. Do you mean this: * For JIRA issue #MATH-1509 Start a PR with "MiniBatchKMeansClusterer", but without the "ClusterUtils", despite the duplicate code between "MiniBatchKMeansClusterer" and "KMeansPlusPlusClusterer", also with "CentroidInitializer" and test code with in a single commit. * Suggestions like "remove the constructors with default parameters" should apply as a new commit of the PR above, and tracking by a subtask of JIRA issue #MATH-1509. * Fire a new JIRA issue for the duplicate code, and start another PR with "ClusterUtils" in, and extract duplicate code into "ClusterUtils". >>"org.apache.commons.math4.userguide.ClusterAlgorithmComparison": >> 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
