emukit icon indicating copy to clipboard operation
emukit copied to clipboard

Add gradient calculation for the covariance between points in GPyModelWrapper

Open BrunoKM opened this issue 4 years ago • 12 comments

This change adds a method get_covariance_between_points_gradients to the GPyModelWrapper to facilitate obtaining the gradients of the corresponding get_covariance_between_points method implemented in the wrapper.

These gradients are useful when optimising any batch acquisition function that relies on the covariance between batch points (such as Multi-point Expected Improvement) sequentially, i.e. when optimising with respect to one element of the batch only.

A second change introduced is vectorization of the gradient calculation for full covariance in the GPyModelWrapper class. Replacing the for-loop with numpy array operations results in ~50% speed-up for larger batches of points (> 50).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

BrunoKM avatar Feb 17 '21 12:02 BrunoKM

Codecov Report

Merging #347 (6738faa) into master (b793989) will increase coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   89.43%   89.46%   +0.02%     
==========================================
  Files         123      123              
  Lines        4051     4061      +10     
  Branches      462      461       -1     
==========================================
+ Hits         3623     3633      +10     
  Misses        332      332              
  Partials       96       96              
Impacted Files Coverage Δ
emukit/model_wrappers/gpy_model_wrappers.py 84.66% <100.00%> (+1.09%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b793989...6738faa. Read the comment docs.

codecov-io avatar Feb 17 '21 12:02 codecov-io

Apologies for taking time on this one, it's a bit hard for me to make complete sense of this change. I promise to get through it!

apaleyes avatar Mar 09 '21 23:03 apaleyes

No worries, let me know if there is anything I can do to explain and document it better in the code.

BrunoKM avatar Mar 10 '21 16:03 BrunoKM

To answer the two points:

  1. This PR introduces a new method that isn't declared in any model interfaces. That's backwards from Emukit's point of view. Are you planning to use this method anywhere? Perhaps create a new acquisition? If so, let's define an interface, or augment an existing one if that makes more sense.

This is indeed intended for an implementation of a new acquisition. It comes in handy whenever optimizing any of many batch acquisition functions sequentially – i.e. by selecting optimising batch acquisition with respect to one point in the batch at a time. Currently, get_covariance_between_points is part of the IEntropySearchModel interface, so maybe something like IDifferentiableEntropySeachModel would make sense?

  1. Both changes carry some pretty heavy matrix algebra, which is really hard to follow. Can you please add some comments throughout, to clarify what's going on? Thanks!

Fair point, I'll try and make it easier to follow.

BrunoKM avatar Apr 11 '21 22:04 BrunoKM

Codecov Report

Merging #347 (c280f6d) into main (cdcb0d0) will decrease coverage by 0.02%. The diff coverage is 93.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #347      +/-   ##
==========================================
- Coverage   89.01%   88.99%   -0.03%     
==========================================
  Files         128      128              
  Lines        4244     4254      +10     
  Branches      609      609              
==========================================
+ Hits         3778     3786       +8     
- Misses        369      371       +2     
  Partials       97       97              
Impacted Files Coverage Δ
emukit/core/interfaces/__init__.py 100.00% <ø> (ø)
emukit/core/interfaces/models.py 61.76% <60.00%> (-0.31%) :arrow_down:
emukit/model_wrappers/gpy_model_wrappers.py 84.24% <100.00%> (+0.55%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cdcb0d0...c280f6d. Read the comment docs.

codecov-commenter avatar Jun 11 '21 12:06 codecov-commenter

What about the interface discussion? Is this resolved @apaleyes ? Where exactly is the new method required in Emukit, i.e., is there an acquisition function that requires the method?

mmahsereci avatar Jun 11 '21 13:06 mmahsereci

Hey @mmahsereci , good to hear from you again!

@BrunoKM promised us an acquisition later down the line. But that's a good point that I forgot all about - the function shall be there as an implementation of some interface. Thanks for catching!

apaleyes avatar Jun 11 '21 13:06 apaleyes

That sounds great, thanks for catching me up @apaleyes! And, yes I am back now it seems. :)

mmahsereci avatar Jun 11 '21 13:06 mmahsereci

@apaleyes sorry, resolving the interfaces has been on the stack of my todos for the past couple of weeks, I will try to get around to it soon.

Yes, this is for a new acquisition that is still in the pipeline :)

BrunoKM avatar Jun 14 '21 11:06 BrunoKM

@BrunoKM are you still planning to work on this PR (and the follow up of it)? Or shall we just close it?

apaleyes avatar Dec 09 '21 20:12 apaleyes

Thanks for the follow-up @apaleyes!

I'm still happy to finish off this PR (if I don't do it before the end-of-year, feel free to close it).

I've been a bit on the fence about whether to push the new acquisition soon afterwards. It's one with particularly tricky gradients: i.e. manually writing a gradient function for it in pure NumPy turned out to be a pain. The current implementation internally uses either jax/pytorch to automatically get the gradient function. I'd assume adding these dependencies is not in line with the philosophy of the rest of the EmuKit repo.

I'm still hoping I'll be able to add the manual gradients later down the line and push the new acquisition function, but due to limited time, I can't say how soon that might be.

BrunoKM avatar Dec 11 '21 09:12 BrunoKM

I added an interface that specifies the covariance between points derivative method. Let me know your thoughts!

As for the new acquisition function, I think I'll be able to add it without an evaluate_with_gradients method relatively soon. An update with evaluate_with_gradients could follow later down the line.

BrunoKM avatar Jan 01 '22 20:01 BrunoKM