Kronecker Gaussian Regression with arbitrarily many sub kernels

[mclaughlin6464]

I modified gp_kronecker_regression object to accept aribtrarily many sub kernels, without ever constructing any of the larger kenels. I had an application I needed this for so I just implemented it. I would also like in the future to implement derivatives with this object as well.

[ekalosak]

Seems like a great contribution - unfortunately not passing tests. Does the change from X1,X2 to Xs cause the .shape assertion fail observed in the CI logs?

[mclaughlin6464]

Ah I see now that I didn't modify the testing scripts to pass in arguments following the new syntax. I will make that adjustment shortly and try the tests again.

[codecov-io]

Codecov Report

Merging #804 into devel will increase coverage by 0.02%.
The diff coverage is 96.61%.

@@            Coverage Diff             @@
##            devel     #804      +/-   ##
==========================================
+ Coverage   54.24%   54.26%   +0.02%     
==========================================
  Files         210      210              
  Lines       21347    21365      +18     
  Branches     2883     2895      +12     
==========================================
+ Hits        11579    11594      +15     
- Misses       9231     9233       +2     
- Partials      537      538       +1

[mclaughlin6464]

ok, should be ready to pull in now.

Possibly worth adding a test to a larger number of kernels than 2 now, if it seems necessary

[ekalosak]

I hate to be that guy, but it's a potential interface change so it's worth picking that nit: the test fixes
@mclaughlin6464 you've pushed (https://github.com/SheffieldML/GPy/pull/804/files#diff-b7384d163820f5bdca24211baa385540R988) appear to change the model's predict() signature. If it were a more obscure function, that might be fine to sweep in, but we probably need a core maintainer's eyes on this now.

[mclaughlin6464]

ok, it is probably possible to remove the [ ] with some *args magic, if that is an important syntactic point, at least for the predict call. The constructor that will likely not be possible, at least not obviously so

[ekalosak]

It's kinda gross, but compliant with the old signature: def __init__(self, X1, X2, Y, kern1, kern2, noise_var=1., name='KGPR', additional_Xs=[]):
Would that work?

[mclaughlin6464]

re the suggestion above one would also needs an additional_kerns=[] as well. I think its quite ugly but I understand the need for consistency, so if this is better for synchronizing with package at large I can implement this change.

[mclaughlin6464]

I added a new test to include coverage of the a 2+ kernel case. I'm glad I did because there seem to be issues. Unfortunately I haven't been able to completely resolve them. Some of the problem appears to be a non-trivial ordering to the input dimensions. However the rest of the issue appears to be compounding numerical issues making equivalence between the normal and kronecker models difficult to assess.

I'll keep working on this but it may take a little more time to get things up to snuff

[ekalosak]

You might consider squashing these commits when you're ready to merge - it will make the master branch's git log cleaner.

[mclaughlin6464]

How do you recommend doing that?

[ekalosak]

Squashing commits can be a bit of a messy thing, but is not fundamentally complicated: this is a good, concise source.
@apaleyes care to weigh in? I wouldn't suggest squashing except a lot of these commits are CI-trigger, small fixes, and typo corrections - which are all great for the codebase, just not for the git commit log.

[apaleyes]

Sure. No need to squash anything, we'll be able to squash everything in a single commit nicely once we merge this PR. if you could just fix those tests it would be great

[codecov-commenter]

Codecov Report

Merging #804 into devel will increase coverage by 0.14%.
The diff coverage is 96.61%.

@@            Coverage Diff             @@
##            devel     #804      +/-   ##
==========================================
+ Coverage   54.12%   54.26%   +0.14%     
==========================================
  Files         211      210       -1     
  Lines       21405    21365      -40     
  Branches     2892     2895       +3     
==========================================
+ Hits        11585    11594       +9     
+ Misses       9271     9233      -38     
+ Partials      549      538      -11     

[lawrennd]

@ekalosak @apaleyes What's the status on this?

I seem to be the only person with power to merge paying attention to repo at the moment. Would you approve merge on this?

[mclaughlin6464]

Apologies, I have had trouble getting the numerics to stabilize enough to path the tests above, and then got distracted by COVID related things. It is still my intent to make this merge but it may take me awhile.

[jbect]

@mclaughlin6464 since this is an old PR, maybe now would be a good time to squash and rebase on top of upstream/devel (where upstream is the central GPy repo).

Here is one way to do it:

git checkout devel

# Undo Neil's last merge
git reset --hard 7a8cfcebfae19075e43f7258e78819a556e7a960

# Squash eveything into one commit
# (use "s" for each commit after the first)
git rebase -i 5a907bd01396a0b66b9547372273adcc3d5d848d

# Rebase on top of upstream/devel (there will be one conflict, easy to solve)
git rebase upstream/devel

# After fixing the conflict:
git rebase --continue

# And finally, force-push to your fork of GPy:
git push -f

[ekalosak]

Leaving this open for now, the content is good but it's not passing tests - still status: work in progress.

[ekalosak]

Closing due to no activity & merge conflicts