-
Notifications
You must be signed in to change notification settings - Fork 435
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
Conjugate Gradient Lower Bound #1706
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1706 +/- ##
===========================================
- Coverage 97.00% 96.97% -0.03%
===========================================
Files 91 92 +1
Lines 4168 4336 +168
===========================================
+ Hits 4043 4205 +162
- Misses 125 131 +6
Continue to review full report at Codecov.
|
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.
LGTM - left some questions and general feedback to improve code quality
k = 100 | ||
xnew = np.linspace(x.min() - 1.0, x.max() + 1.0, k).reshape(-1, 1) | ||
|
||
pred_no_tol = cglb.predict_y(xnew, cg_tolerance=None) |
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.
Does this mean no tolerance or cached tolerance, which I believe to be 1?
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 means that no conjugate gradient will be run.
The cached value of self.aux_vec
will be used. If self.aux_vec=0
, e.g. when the model is initialized, prediction will match SGPR
. This is what is currently in the docstring for predict_f
:
cg_tolerance: float or None: If None, the cached value of :math:
v is used. If float, conjugate gradient is run until :math:
rᵀQ⁻¹r < ϵ.
Any suggestions on things we could do to make this clearer? Would it be useful to add this description to the notebook?
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.
@DavidBurt2, we should just re-state the same in the notebook.
Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com>
Co-authored-by: Vincent Dutordoir <dutordoirv@gmail.com>
CGLB [1] has a tighter bound and better hyperparameter estimation comparing to SGPR model. Implementation-wise the difference between SGPR and CGLB is an extra code for the conjugate gradient, which is quite simple. The original implementation can be found at [2].
[1] https://arxiv.org/abs/2102.08314v1
[2] https://github.com/awav/CGLB
TODO list:
@DavidBurt2