Skip to content
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

Enable mean-centering #1482

Merged
merged 2 commits into from Aug 6, 2018

Conversation

Projects
None yet
2 participants
@manish7294
Copy link
Member

manish7294 commented Jul 31, 2018

This PR includes code for performing mean-centering on the dataset.

@rcurtin
Copy link
Member

rcurtin left a comment

Looks good to me. If you want to consider adding another option for saving the centered data I think it's good to merge. 👍

PRINT_PARAM_STRING("normalize") + " parameter. "
PRINT_PARAM_STRING("normalize") + " parameter. Furthermore, " +
"mean-centering can be performed on the dataset by specifying the " +
PRINT_PARAM_STRING("center") + "parameter. "

This comment has been minimized.

@rcurtin

rcurtin Aug 2, 2018

Member

Actually I realized that there's one more thing, which is that if we learn a metric using a mean-centered dataset, we can't then apply that metric to the non-mean-centered dataset. So we should also add an output parameter centered_data or something like this, so that a user can have access to their mean-centered data for later use. What do you think?

This comment has been minimized.

@manish7294

manish7294 Aug 2, 2018

Author Member

Fair enough!

@@ -136,6 +138,8 @@ PARAM_INT_IN("rank", "Rank of distance matrix to be optimized. ", "A", 0);
PARAM_FLAG("normalize", "Use a normalized starting point for optimization. It"
"is useful for when points are far apart, or when SGD is returning NaN.",
"N");
PARAM_FLAG("center", "Perform mean-centering on the dataset. It is useful "
"when points have large norm values.", "C");

This comment has been minimized.

@rcurtin

rcurtin Aug 2, 2018

Member

I guess technically it would be when the centroid of the data is far from the origin.

This comment has been minimized.

@manish7294

manish7294 Aug 2, 2018

Author Member

Right, that should be the one. Thanks for pointing this out.

@rcurtin

rcurtin approved these changes Aug 2, 2018

Copy link
Member

rcurtin left a comment

Looks great to me. I'll go ahead and merge this in 3 days to leave time for any other comments. Thanks! 👍

I think that the LMNN binding is quite useful now. Great work overall. :)

@rcurtin rcurtin merged commit 6c992fe into mlpack:master Aug 6, 2018

5 checks passed

Memory Checks Build finished.
Details
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.

Copy link
Member

rcurtin commented Aug 6, 2018

Thanks again! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.