Quadrature Refactoring #1505
Merged
Quadrature Refactoring #1505
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vdutor
reviewed
Jul 20, 2020
There was a problem hiding this comment.
Hi @gustavocmv, thanks for your amazing PR and apologies for the long-overdue review - let's try to get this PR merged this week. I left you a couple of minor comments. Would you mind following Ti's suggestion and moving the old quadrature code to the tests. We will also need to update the code base to use the new quadrature code.
Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com>
vdutor
reviewed
Jul 21, 2020
vdutor
approved these changes
Jul 30, 2020
7 tasks
7 tasks
vdutor
added a commit
that referenced
this pull request
Aug 27, 2020
* Update pull request template (#1510) Clarify template to make it easier for contributors to fill in relevant information. * Temporary workaround for tensorflow_probability dependency issue (#1522) * pin cloudpickle==1.3.0 as temporary workaround for tensorflow/probability#991 to unblock our build (to be reverted once fixed upstream) * Update readme with new project using GPflow (#1530) * fix bug in varying_noise notebook (#1526) * Fix formatting in docs (intro.md) and restore link removed by #1498 (#1520) * pin tensorflow<2.3 tensorflow-probability<0.11 (#1537) * Quadrature Refactoring (#1505) * WIP: quadrature refactoring * Removing old ndiagquad code * deleted test code * formatting and type-hint * merge modules * black formatting * formatting * solving failing tests * fixing failing tests * fixes * adapting tests for new syntax, keeping numerical behavior * black formatting * remove printf * changed code for compiled tf compatibility * black * restored to original version * undoing changes * renaming * renaming * renaming * reshape kwargs * quadrature along axis=-2, simplified broadcasting * black * docs * docs * helper function * docstrings and typing * added new and old quadrature equivalence tests * black * Removing comments Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com> * Typo Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com> * notation Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com> * reshape_Z_dZ return docstring fix * FIX: quad_old computed with the ndiagquad_old Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com> * more readable implementation Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com> * tf.ensure_shape added * removed ndiagquad * removed ndiagquad * Revert "removed ndiagquad" This reverts commit 7bb0e9f. * FIX: shape checking of dZ * Revert "removed ndiagquad" This reverts commit 8e23524. Co-authored-by: Gustavo Carvalho <gustavo.carvalho@delfosim.com> Co-authored-by: ST John <st@prowler.io> Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com> * Add base_conditional_with_lm function (#1528) * Added base_conditional_with_lm function, which accepts Lm instead of Kmm Co-authored-by: Neil Ferguson <neil@prowler.io> Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com> Co-authored-by: st-- <st--@users.noreply.github.com> * Fixed separate_independent_conditional to correctly handle q_sqrt=None. (#1533) * Fixed separate_independent_conditional to correctly handle q_sqrt=None. Co-authored-by: Aidan Scannell <scannell.aidan@gmail.com> Co-authored-by: st-- <st--@users.noreply.github.com> * Bump version numbers to 2.1.0. (#1544) * Re-introduce pytest-xdist (#1541) Enables pytest-xdist for locally running tests (`make test`) on multiple cores in parallel. * check dependency versions are valid on CI (#1536) * Update to not use custom image (#1545) * Update to not use custom image * Add test requirements * Update parameter to be savable (#1518) * Fix for quadrature failure mode when autograph was set to False (#1548) * Fix and test * Change shape of quadrature tensors for better broadcasting (#1542) * using the first dimension to hold the quadrature summation * adapting ndiagquad wrapper * Changed bf for bX in docstrings Co-authored-by: Gustavo Carvalho <gustavo.carvalho@delfosim.com> Co-authored-by: st-- <st--@users.noreply.github.com> Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com> * Update min TFP supported version to 0.10 (#1551) * Broadcasting constant and zero mean function (#1550) * Broadcasting constant and zero mean function * Use rank instead of ndim Co-authored-by: st-- <st--@users.noreply.github.com> Co-authored-by: joelberkeley-pio <joel.berkeley@prowler.io> Co-authored-by: gustavocmv <47801305+gustavocmv@users.noreply.github.com> Co-authored-by: Gustavo Carvalho <gustavo.carvalho@delfosim.com> Co-authored-by: ST John <st@prowler.io> Co-authored-by: Neil Ferguson <nfergu@users.noreply.github.com> Co-authored-by: Neil Ferguson <neil@prowler.io> Co-authored-by: Aidan Scannell <as12528@my.bristol.ac.uk> Co-authored-by: Aidan Scannell <scannell.aidan@gmail.com> Co-authored-by: Sandeep Tailor <s.tailor@insysion.net> Co-authored-by: Artem Artemev <art.art.v@gmail.com>
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Goals
This PR refactors the
ndiagquadfunction responsible for performing the quadrature needed to compute expectations related to non-conjugate likelihoods.The new implementation uses classes to implement quadrature methods. This enables the class to store the points and weights used for quadrature. It also opens the possibility of componentizing the quadrature class inside the likelihood using it, allowing for operations such as training with some quadrature algorithm (e.g. Gauss-Hermite) and predicting with another (e.g. Monte Carlo).
It also clears the quadrature function signature. Instead of expecting lists Fmu and Fvar representing multiple latent GP's, it now takes the tensors as arguments, with the latent GPs stacked in the last dimension.
Design
The new quadrature class design can be summarized as follows:
GaussianQuadratureabstract base class, which provides the methods for computing expectations (or log-expections-exp) given points X and weights W, which must be provided by the inheriting class nNDiagGHQuadratureclass, which inherits fromGaussianQuadrature, and implements the construction of the points X and weights W with the methodThis separation aims to modularize these concepts and provide better extension capacity.
Backward Compatibility
Due to the migration to a class architecture, the old
gpflow/quadrature.pyfile was converted to a submodule in the foldergpflow/quadrature, with it's contents moved togpflow/quadrature/deprecated.py.For backward compatibility, The
ndiagquadfunction was refactored to wrap the newNDiagGHQuadratureclass.The old code was not deleted -- it was moved to
gpflow/quadrature/old.pyand can be accessed withgpflow.quadrature.ndiagquad_old. @st-- suggested moving it to thetestsand build tests to compare both implementations.