To me the maintainability of the added code also plays a role, and this PR is really nice and short in its implementation.
It needs better documentation (other than the plot_ file) to better demonstrate its benefits, otherwise looks reasonable to have it IMO. On Tue, Feb 26, 2019 at 11:11 AM Gael Varoquaux < [email protected]> wrote: > I need core devs opinion (please, only core devs, I am sending this on > the public ML for transparency): > > The following PR adds a speed up for expansion of polynomial kernels: > https://github.com/scikit-learn/scikit-learn/pull/13003 > > According to the author, the speed up is significant (needs to be > verified during a code review). > > The paper is a bit below citation level for inclusion of a new method, > however this can be seen as a speed up of Nystrom. Strictly speaking, it > is not just a speed-up, as it introduces a new estimator. > > The discussion on the PR is short and quickly reviews the relevant > literature. > > > My question: should we consider this as acceptable for inclusion > (provided that it does show significant speedups with good prediction > accuracy)? I am asking to know if we start the review and inclusion > process or not. > > > Cheers, > > > Gaël > _______________________________________________ > scikit-learn mailing list > [email protected] > https://mail.python.org/mailman/listinfo/scikit-learn >
_______________________________________________ scikit-learn mailing list [email protected] https://mail.python.org/mailman/listinfo/scikit-learn
