Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Some improvement and additions to MO kernels #354
Some improvement and additions to MO kernels #354
Changes from 24 commits
8427ae5
1a5ac23
d79d24b
4f23240
a33c312
a729c01
e1f92a8
e7d7db9
ee69c6e
d1d8d45
d80453c
06cbb3d
d03b951
c8f8093
cd6dedc
8b7410a
1978e25
3ffd436
c42e43d
96bf55e
b649e65
fe81b0a
509dfd6
33f3e87
0f9ce64
51a3736
8459d0c
1675598
0672a1f
cf8e8f3
31620e2
9b27b58
d68f86e
59ff2c8
7c99633
286d5d5
cb37e0f
fee8eaf
3466daa
533f8fd
213b606
f2c6ae6
407ca80
6bcc83d
d5571df
13427a7
48db5a0
43a5ab9
f2ac5ed
dac5db7
e61a668
8cbb1eb
c72efd9
11e6d56
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this be separate from
iskroncompatible
? If it's not possible to reuseiskroncompatible
, could you add a docstring explaining why this one is different (and how so)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I did this because I wanted to avoid confusion with the first two functions, because the MO kernel matrix construction is distinct from the kronecker construction that was there already.
I realize now that I should have used a different name than
kernelkronmat
for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kernelkronmat
seems fine to me - it's basically saying "please give me a lazy Kronecker" whether it's because we have outputs-as-inputs or seperable-on-grid-of-features or something elseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be (somewhat) less efficient than separate implementations that then only call
kernelmatrix(k.kernel, x.x)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is, do we want separate methods for the sake of optimal efficiency? @willtebbutt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be nice, but I'm not going to insist on it going in in this PR provided it gets sorted in a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Crown421 if you don't want to do it in this PR, please open a new issue for it instead then: )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's starting to get a bit confusing with _kronkernelmatrix, _kroneckerkernelmatrix, kernelkronmat... how about at least clarifying in the method name that this is multioutput-specific? e.g.
(moving the argument on which we actually dispatch to the front)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite happy with this being a different pattern from kernelkronmat when they should otherwise be the same,
kron
vsKronecker
being the only difference. Could it be a single method that returns either dense kron() or lazy Kronecker? If that would lead to type instability issues, could dispatch on a Val{} maybe? lazy=Val{true} vs lazy=Val{false} (it's just an internal function anyways)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @st-- regarding argument names in particular --
Kfeatures
andKoutputs
are quite a bit more informative thanKtmp
andB
. The function name also makes more sense to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor suggestion:
doesn't make any difference as
B
must be square, but then it's more consistent with the LMC one:)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this not turn into an infinite loop ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the inner function has/ requires a kwarg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, didn't realize dispatch depended on kwargs as well, I thought it was only on the positional args...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to be a bug (JuliaLang/julia#9498) that is only not fixed yet because it would break backwards compatibility - I would prefer not to abuse implementation details that go counter to what the Julia docs say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the layers of indirection, how about
etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I have been doing this for a while, good to know that it will go away at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately,
I
does not have a size. The current implementation of theIndependentMOKernel
does not have a output dimension attached, even though I personally would prefer it to be a special case of the theIntrinsicCoregionMOKernel
whereB = Matrix(I, out_dim, out_dim)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to be consistent with the MOInput* types and call it
out_dim
insteadThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: line length
Additionally, it would be helpful to add a doctest here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have unfortunately never used documenter, will need to read up on this. Style should be fixed by formatter now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, how about instead just having
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x
andy
could be types on whichsize
isn't defined (e.g. a string)