-
Notifications
You must be signed in to change notification settings - Fork 5
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
Cross Validation Refactor #45
Conversation
|
||
RegressionDataset(){}; | ||
|
||
RegressionDataset(const std::vector<FeatureType> &features_, |
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.
the trailing underscore convention is backwards here from what we normally have. I'm going to assume its consistent with the rest of albatross however which is all that really matters. I think we talked about this ages ago... :)
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.
yeah, the rest of albatross uses a trailing underscore to avoid shadowing. I started to try to change them all ... then decided it was more work than it was worth.
/* | ||
* Extracts a subset of columns from an Eigen::Matrix | ||
*/ | ||
template <typename SizeType> |
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 have to be templated? can it not just be hardcoded to use size_t
or unsigned
or whatever?
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.
Yeah this might be overuse of templating, in general the value type of col_indices
can be anything that can be converted to an Eigen::Index
, but now I think the only thing that is really used is s32
. Whatever the decision is the same argument would apply to all the rest of the subset
functions in this file as well.
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.
quick glance over, looks good to me. I will come back and check it out more thoroughly when i have a sec.
e504b58
to
c0779fe
Compare
c0779fe
to
86e9468
Compare
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
Moves
cross_validated_predictions
to a method inRegressionModel
which allows specialization of the cross validation. The only real change in behavior is the ability to obtain a cross validatedMarginalDistribution
. The existing code only allows for the cross validated mean.The primary reason for this reorganization is to be able to have per model specialization of some of the cross validation routines. For example
gp_fast_loo_nll
which takes advantage of a linear algebra trick to accelerate leave one out predictions for Gaussian processes is now directly available using theRegressionModel
api (instead of the current static cast hack.)The first commit is just a lot of moving things around, the second contains the
RegressionModel
api changes. A followup PR will switchgp_fast_loo_nll
to be a specialized method ingp.h
.