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

Add k-fold cross-validation #1095

Merged
merged 15 commits into from Aug 22, 2017

Conversation

Projects
None yet
3 participants
@micyril
Member

micyril commented Aug 15, 2017

See the previous discussion here.

@rcurtin

Since we've already discussed this in your repo while we waited to merge other parts, I think this is ready to go. Great work---I think this is high-quality code and look forward to using it in practice.

Since this is a little complex and I'm insta-approving it, let's go ahead and wait a week before merging it in, in case anyone else has comments.

@zoq

zoq approved these changes Aug 15, 2017

Well written, great work!

@micyril

This comment has been minimized.

Show comment
Hide comment
@micyril

micyril Aug 16, 2017

Member

Thank you for your reviews. I would like to mention one more thing that came to my mind. For the case k=2 we can actually avoid data copying if the user have passed moved data (r-value references). I don't think that k-fold cross-validation is really intended to be used with k=2, and this implementation will make constructors seem more "heavier" since they will be templated, but anyway we can do that. Let me know if you would like me to add the change.

Member

micyril commented Aug 16, 2017

Thank you for your reviews. I would like to mention one more thing that came to my mind. For the case k=2 we can actually avoid data copying if the user have passed moved data (r-value references). I don't think that k-fold cross-validation is really intended to be used with k=2, and this implementation will make constructors seem more "heavier" since they will be templated, but anyway we can do that. Let me know if you would like me to add the change.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Aug 16, 2017

Member

Personally I think that the set of people using k = 2 is so small it's not worth optimizing for that use case. So my opinion would be to leave the code as it is. Of course, if you want to change the code to handle that case, I don't have any problem with that, but it is up to you.

Member

rcurtin commented Aug 16, 2017

Personally I think that the set of people using k = 2 is so small it's not worth optimizing for that use case. So my opinion would be to leave the code as it is. Of course, if you want to change the code to handle that case, I don't have any problem with that, but it is up to you.

@micyril

This comment has been minimized.

Show comment
Hide comment
@micyril

micyril Aug 19, 2017

Member

Let's then leave it as it is.

Member

micyril commented Aug 19, 2017

Let's then leave it as it is.

@rcurtin rcurtin merged commit 879bd46 into mlpack:master Aug 22, 2017

4 checks passed

Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Aug 22, 2017

Member

Great work! Glad to merge it. :)

Member

rcurtin commented Aug 22, 2017

Great work! Glad to merge it. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment