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

QDA and LDA using R. #103

Merged
merged 2 commits into from Aug 17, 2017

Conversation

Projects
None yet
3 participants
@Iron-Stark
Contributor

Iron-Stark commented Aug 14, 2017

No description provided.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Aug 15, 2017

Member

Do you want me to wait to review this until you have added all of the methods you will be adding from R?

Member

rcurtin commented Aug 15, 2017

Do you want me to wait to review this until you have added all of the methods you will be adding from R?

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Aug 16, 2017

Contributor

@rcurtin
I will add other methods in different PR's. This is ready for review.

Contributor

Iron-Stark commented Aug 16, 2017

@rcurtin
I will add other methods in different PR's. This is ready for review.

Show outdated Hide outdated methods/R/lda.py
Show outdated Hide outdated methods/R/lda.r
toc(log = TRUE)
out <- capture.output(tic.log(format = TRUE))
cat(out, file="log.txt", append=FALSE)

This comment has been minimized.

@zoq

zoq Aug 16, 2017

Member

Do we have to remove the filer later on?

@zoq

zoq Aug 16, 2017

Member

Do we have to remove the filer later on?

Show outdated Hide outdated tests/benchmark_qda.py
@rcurtin

Looks good to me; only a couple minor comments.

install.packages('tictoc', repos='http://cran.us.r-project.org')
# Read the command line arguments in a vector.
library(mlr)

This comment has been minimized.

@rcurtin

rcurtin Aug 16, 2017

Member

It looks like the mass package also has an implementation of LDA. Have you looked into that? Do you know which is better to use here?

@rcurtin

rcurtin Aug 16, 2017

Member

It looks like the mass package also has an implementation of LDA. Have you looked into that? Do you know which is better to use here?

This comment has been minimized.

@rcurtin

rcurtin Aug 16, 2017

Member

Actually it looks like mlr uses mass internally, so nevermind my comment here.

@rcurtin

rcurtin Aug 16, 2017

Member

Actually it looks like mlr uses mass internally, so nevermind my comment here.

Show outdated Hide outdated tests/benchmark_qda.py

@zoq zoq merged commit b3d2477 into mlpack:master Aug 17, 2017

1 check passed

Benchmarks Checks Build finished.
Details
@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 17, 2017

Member

Thanks for the quick fix.

Member

zoq commented Aug 17, 2017

Thanks for the quick fix.

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