Closed patel-zeel closed 2 years ago
Hey @patel-zeel, thanks a lot for putting this together!! Sorry for not getting back earlier to this—I was preoccupied with the AISTATS deadline. Just running the tests now. I'll get back to this with more detailed feedback in the next few days, but on a skim this looks good and basically ready to go. :)
Totals | |
---|---|
Change from base Build 1407415532: | 0.0% |
Covered Lines: | 877 |
Relevant Lines: | 877 |
No problem at all @wesselb. Thank you for getting back and agreeing for the feedback :)
Also, if you like, feel free to add an entry to the README which documents the FITC approximation. (I'm also happy to do that myself.)
Hi @wesselb,
Sorry for returning to this after a long time.
I have made the following changes:
PseudoObservations
as a parent class for PseudoObsVFE
and PseudoObsFITC
.@pytest.mark.parametrize
for PseudoObsVFE
and PseudoObsFITC
.example*.py
and README to incorporate change from PseudoObsVFE
to PseudoObs
Please let me know wherever minor/major changes are required.
Hey @patel-zeel, absolutely no rush! This is looking great now. :)
Only one comment really: it might not be a good idea to rename PseudoObs
to PseudoObsVFE
, since that would break existing code depending on the name PseudoObs
. I therefore suggest the following renaming:
PseudoObservations
-> AbstractPseudoObservations
(to avoid a clash with naming PseudoObservationsVFE
back to PseudoObservations
)PseudoObservationsVFE
-> PseudoObservations
With those changed, this seems ready to go! I really appreciate the contribution. :)
Hey @wesselb,
Your suggestion makes perfect sense. I have applied it to the relevant places.
Thank you for your iterative suggestions on this PR. It has been a really good learning experience for me :)
Thank you @patel-zeel! And no problem at all. :)
This is really great! Your contribution is really appreciated. Super happy to have another approximation implemented.
I've merged the PR. I will release another version of the package in the next few days which includes the FITC approximation. :)
Sounds great! Thank you @wesselb :)
Added main class and tests for FITC sparse regression.