[MRG]Allow Gaussian process kernels on structured data #13936 #13954
Conversation
|
Thanks for the pr.
I don't think we currently have any examples stored in ipynb, even if they
export to ipynb. Apart from anything else, it is a nuisance with version
control. Please look at our other examples and format similarly.
|
ae89b25 to
8fb6e85
Compare
43a7d2b to
21e3645
Compare
Done. Thanks for the comment! |
21e3645 to
3da4e6f
Compare
|
Please avoid force-pushing. It makes it quite difficult to see changes, and risks overwriting core devs' tweaks to your work. Instead, always append commits, and we will squash upon merge. |
I see. Got it. |
|
@jnothman Could you please review it and let me know if there are anything to be resolved? |
When I find time for it, yes! |
There was a problem hiding this comment.
Sorry for the delay. I find this quite nice, thank you.
Please add an entry to the change log at doc/whats_new/v0.22.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:
| feature vector form. This is enabled through the use of kernel functions | ||
| that can directly operate on discrete structures such as variable-length | ||
| sequences, trees, and graphs. | ||
| """ |
There was a problem hiding this comment.
I think it would help to briefly describe the kernel, and to also describe the plots. It's unclear, for instance, that you do not include two samples in training in the Regression example.
| @@ -355,6 +356,10 @@ def diag(self, X): | |||
| def is_stationary(self): | |||
| """Returns whether the kernel is stationary. """ | |||
|
|
|||
| @abstractmethod | |||
There was a problem hiding this comment.
I'm not entirely comfortable doing this as it will break any custom kernels users have created... they will not be able to construct their kernel.
Could we instead default to true for the sake of backwards compatibility?
Also, should this be a method or a property?
There was a problem hiding this comment.
I made it defaults to True, and removed VectorKernelMixin because it is now redundant. I think it is fine either as a method or property, but what confuses me is that is_stationary is a method instead of a property. Is there some particular thought behind this?
| kernel.kernel.requires_vector_input()) | ||
| if isinstance(kernel, KernelOperator): | ||
| assert_equal(kernel.requires_vector_input(), | ||
| np.any([kernel.k1.requires_vector_input(), |
There was a problem hiding this comment.
why not just use or instead of np.any??
| @@ -636,6 +671,11 @@ def is_stationary(self): | |||
| """Returns whether the kernel is stationary. """ | |||
| return self.k1.is_stationary() and self.k2.is_stationary() | |||
|
|
|||
| def requires_vector_input(self): | |||
| """Returns whether the kernel is stationary. """ | |||
| return np.any([self.k1.requires_vector_input(), | |||
There was a problem hiding this comment.
why not use or instead of np.any?
|
Please merge in master and resolve conflicts with it. And ask for help if you're having trouble |
|
Will do. Working on it right now.
…On Mon, Aug 5, 2019 at 4:16 PM Joel Nothman ***@***.***> wrote:
Please merge in master and resolve conflicts with it. And ask for help if
you're having trouble
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#13954?email_source=notifications&email_token=ABOMX5DZKIHBPFRXSXFM623QDCYGRA5CNFSM4HPXTAC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3TLD6A#issuecomment-518435320>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABOMX5FHUO6B7Q4FJXQBXDDQDCYGRANCNFSM4HPXTACQ>
.
|
…ctured data in addition to fixed-length feature vectors. Made the Gaussian process regressor and classifier compatible with either structure- or vector-based kernels.
…ifferent check_array/check_X_y calls
convention-conforming variable names Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>
convention-conforming variable names Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>
Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>
3d4503b to
29978c3
Compare
|
It's been a while since last time and it made rebasing somehow difficult. As such, I've started from scratch on a different branch and created a new PR #15557 to keep things clean. @jnothman I've incorporated all your suggestions in the new branch/PR. Could we keep subsequent discussions in the new PR? Thanks! |
Fixes #13936.
Made the Gaussian process regressor and classifier compatible with either structure- or vector-based kernels. Specifically:
vector_X_only()abstract method to the GP kernel base class.VectorOnlyKernelMixinandStructureOrGenericKernelMixinbase classes to overload thevector_X_only()method for kernels that can only operate on fixed-length feature vectors and that can work on structured and/or generic data, respectively.check_arrayandcheck_X_yparameters for vector/structured input. I.e., do not force the X and Y array to be at least 2D and numeric if the kernel can operate on structured input. Updated documentations accordingly.'plot_gpr_on_structured_data.py.