This PR might be interesting for @dubourg, @agramfort, @larsmans, @JohnFNovak, and @AlexanderFabisch

Several things here:

1. While we are refactoring, is there a way we can deprecate the term "nugget"? I really, really dislike the name - there has to be some clearer title for general users.
2. I don't know about the two abstract base classes. On the one hand, I see the logic in doing it in this way, but I also feel that explicit independent functions for each make things more modular (which is the reasoning behind the change), and would lend a hand toward using these kernels in an eventual KernelMixin class, which was discussed by @mblondel somewhere around here. At very least, I currently don't see why overriding call is preferable to simple making that functionality live in .transform() or another method, and setting the hyperparameters on fit().

Actually, I am not 100% sure why these classes need all the fit() machinery/stateful behavior at all. It seems like there isn't much need for the class structure, and could be implemented as pure functions that also use some shared functions from the module level. Maybe I am missing something crucial?

regarding 1) I also dislike nugget, but it is part of the external GaussianProcess API so I didn't change it.
regarding 2) I am open to use an other interface than call; I mainly used call because it allows to use the correlation model in a similar way as the old functional correlation models . I don't know about calling it transform; it is not really transforming the data and can even be called without data X (computing the autocorrelation on self.X). Regarding splitting the abstract base class into several independent functions: I am not sure if this could easily be passed to the GP via the corr parameter of the constructor?

On the first, I am glad to see that I am not the only person who is bugged by the name :) . It would be a solid opportunity to deprecate nugget at the API level, then introduce a new parameter that we expose as well and use internally, though I am not sure of a good name yet.

I will try to work up a skeleton example off your branch to show what I mean on the second count. Thanks for the work so far - GP definitely needs some love!

Regarding the fit() machinery/stateful behavior: The l1_cross_differences stored in StationaryCorrelation.D need to be computed only once. Formerly, they have been stored in the GP itself (where they are not really required). The stateful behavior would also be required for implementing some non-stationary correlation models from the literature (not part of this PR).

On the l1_cross_differences, it would be pretty easy to do an "if None" check to avoid recomputing.

I would actually prefer storing state in the GP itself - if that was the case, we could still use the state stored in the GP to implement non-stationary correlation models, right? It seems more explicit to me to keep the state in the class itself, and let the correlations stay as simple as possible. In any case, I will try and work out an example of "stupid" correlations off your branch to see if it is a reasonable approach.

Ultimately, I think my comments are better addressed by a larger refactor of the GP class. Ignore me for now, and we can analyze this PR on it's own merits.

In general we should work toward making the GP class more transparent and accessible - I currently find it quite complicated for (possibly) no reason, and much of the class does not adhere to modern sklearn API standards.

Adding a class hierarchy to the kernel methods which is not shared with other kernel classes, pairwise_metrics, etc. doesn't really help this. If we decide to merge this PR, I would prefer to see these implementation details (the two metaclasses and their subclasses) private rather than public, in order to make a larger refactor easier in the future. Making the correlations/kernels/covariances very dumb and keeping as much state contained to the GP class as possible is also a step in this direction. That said, I think this PR can definitely help people work in the GP class now, which is very important.

There is a lot in common between kernel ridge regression and GP/kriging - I do not advocate writing "the class to end all classes" but it is definitely worth seeing if we can merge some common functionality now and in the future.

This is a big PR, let's not start changing the API ("nugget" vs. whatever) if we don't need to. Too many good PRs go stale because to many changes are introduced at the same time and the review process becomes a nightmare.

Good point. If it helps get this merged, let's minimize the surface area
and go with what it does now. I think it is very good, but I am unhappy
with the GP in general. This should help that :)

On Tue, Jul 15, 2014 at 12:52 PM, Lars Buitinck notifications@github.com
wrote:

This is a big PR, let's not start changing the API ("nugget" vs. whatever)
if we don't need to. Too many good PRs go stale because to many changes are
introduced at the same time and the review process becomes a nightmare.

—
Reply to this email directly or view it on GitHub
#3388 (comment)
.

+1 that the GP class needs to get more transparent and accessible
In my opinion, the best way for this is to focus on one part of it at a time (probably this PR is already too big). Maybe we can leave the ML vs. MAP part for later and focus first on getting the correlation models straight. Here my thoughts on this:

• correlation_models vs. pairwise_metrics: the strongest overlap here is in my opinion the function l1_cross_differences which is pretty similar to manhattan_distances from pairwise (for Y==None). The main difference seems to be that l1_cross_differences exploits symmetry of D (when Y==None) and has thus a smaller memory footprint. Another overlap is between SquaredExponential and the rbf_kernel from pairwise. rbf_kernel is restricted to the isometric case at the moment, however.
• correlation models as functions versus stateful objects: for the correlation models implemented currently in sklearn, the state consists basically of the pairwise distances of the training data (via D and ij). This could be kept in the GaussianProcess class. I see many use cases for custom correlation models (and have some myself), which need not become part of sklearn itself. It would be sufficient to keep them external but be able to use them with sklearn's GP as long as they have a compatible interface with sklearn's correlation models. Since some kernels might not only differ in computation but also in their internal state (one of my examples stores a clustering of X internally), stateful objects are in my opinion better suited for this. The specific interface of StationaryCorrelation is certainly not optimal; I am open for suggestions ;-)

Is there any way we could separate the bugfix stuff and the refactoring/API change proposal into two PRs? I really, really like the bugfixes but we will need to have a lot of thought and input about the API stuff. If we can split this into two smaller PRs I think it is more likely to get looked at.

I will create a separate PR for the bugfixes and corresponding tests

Great - there are a lot of fixes here :)

Oops, seems that we fixed the same issue twice. In order to get PR #2632 finally merged, I added a proposal for a regression test in the comments.

Now that the bugfixes are merged via PR #3545, what is the general opinion regarding the new features? And using classes for the correlation_models?

If this PR is too big, I can focus on a subset of the features/refacotoring and create a smaller PR.

If have created a separate PR (PR #3885) for adding the Matern kernel to pairwise.py. Once that is done, I will try refactoring correlation_models.py such that it uses as much from pairwise.py as possible and close this PR. What is your opinion, @kastnerkyle?

That sounds awesome! Thanks for all your work on this, it is really nice to
see the GP getting some fixes - sharing the kernels via pairwise.py should
be good for many estimators. It will also provide a clear roadmap for
people wanting to use their own kernels.

On Fri, Dec 19, 2014 at 9:57 AM, Jan Hendrik Metzen <
notifications@github.com> wrote:

If have created a separate PR (PR #3885
#3885) for adding the
Matern kernel to pairwise.py. Once that is done, I will try refactoring
correlation_models.py such that it uses as much from pairwise.py as
possible and close this PR. What is your opinion, @kastnerkyle
https://github.com/kastnerkyle?

—
Reply to this email directly or view it on GitHub
#3388 (comment)
.

@jmetzen you closed this becaue you want to break it up? Sorry for the lack of feedback before.

I plan to split the contents of this PR into several smaller PRs. First step would be to refactor correlation_models.py such that it reuses the kernels from pairwise.py.

Btw, did you follow the (somewhat) recent discussion on the mailing list on whether it would be worth to have a language for specifying kernels in sklearn?
When using the pairwise model we might end up with a lot of redundant computation if we combine many kernels, right?
I guess it would be better than what we currently have, though.

I suspsect that we'll need kernel classes anyway for kernel parameter
optimization by gradient descent. Also popular kernels tend to be different
for SVMs and GPs.

So if we need kernel classes, I'm not sure the proposed refactoring is the best idea.

My advice would be to worry about code factorization after the GP module got rewritten.

Makes sense to defer refactoring until the GP module is rewritten. I also checked how pairwise's kernels could be reused in the GP context and there are at least 2 drawbacks: pairwise's kernels are restricted to the isotropic case (scalar gamma) and they don't exploit that k(X, X) is symmetric - in contrast to the current GP implementation. Thus, code reuse between pairwise and correlation_models would probably also require some non-trivial refactoring of pairwise.

BTW: Is anyone currently working on the GP refactoring?

Unfortunately not.
@dfm said he might have some time to help with improving documentation, but he seems busy finding a new earth ;)