Hello.
Le mer. 26 févr. 2020 à 03:49, CT <chentao...@qq.com> a écrit :
>
> 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.
This last mail of yours was received by the mailing list, as witnessed
by the archive copy:
https://markmail.org/message/qjk6zmdypqigxl2k
Hitting "reply" in my mail client now works as expected (setting the
destination to the ML).
However, you sent the same post twice. And the quoted text is
somewhat mangled again by the quotation mark being replaced
by the corresponding HTML entity (">").
> >>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".
No, you should start with the smallest possible self-contained PR.
For example, why should we commit a code that defines several
constructors, while we already know that a second commit should
remove them?
As you've noticed that some functionality must be factored out of
"KMeansPlusPlusClusterer", this should be done first as a separate
JIRA issue. IIUC, you propose "ClusterUtils". By reviewing a
minimal PR, we should be able to examine whether another
approach might be better (than a "utility" class) in order to expose
functionality common to all clusterer algorithms.
For example, could all "Kmeans" implementations inherit from
a common base class?
Best regards,
Gilles
> >>"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
