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

Install Script for R #102

Merged
merged 5 commits into from Aug 14, 2017

Conversation

Projects
None yet
3 participants
@Iron-Stark
Contributor

Iron-Stark commented Aug 11, 2017

No description provided.

@rcurtin

Looks good to me. I don't know if you meant to include the NBC code in this PR too, but I think it's ok. You might consider doing work on different branches to avoid this in the future.

Once this passes the tests I think it's ready; no other comments from my end. We'll have to re-run the benchmarks checkout job after the merge to get R installed.

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Aug 14, 2017

Contributor

@rcurtin

Actually I was thinking about adding all the R implementations to this PR itself. Please let me know if you think that we need to create a different PR for the implementations.

Contributor

Iron-Stark commented Aug 14, 2017

@rcurtin

Actually I was thinking about adding all the R implementations to this PR itself. Please let me know if you think that we need to create a different PR for the implementations.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 14, 2017

Member

We should open a new PR for the implementations, right now we rebuild everything from scratch, since the PR modified the build script, which is slow.

Member

zoq commented Aug 14, 2017

We should open a new PR for the implementations, right now we rebuild everything from scratch, since the PR modified the build script, which is slow.

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Aug 14, 2017

Contributor

@zoq

Sure. I'll do that for the other implementations. I was about to merge the QDA script too. Now I'll wait for this to merge and then will do the implementations in another PR. Should I remove the NBC ones from here or is it fine?

Contributor

Iron-Stark commented Aug 14, 2017

@zoq

Sure. I'll do that for the other implementations. I was about to merge the QDA script too. Now I'll wait for this to merge and then will do the implementations in another PR. Should I remove the NBC ones from here or is it fine?

@zoq

Just pointed out some minor issues.

@@ -0,0 +1,43 @@
install.packages('mlr', repos='http://cran.us.r-project.org')

This comment has been minimized.

@zoq

zoq Aug 14, 2017

Member

Not sure, but it looks like the package isn't needed for the NBC method?

@zoq

zoq Aug 14, 2017

Member

Not sure, but it looks like the package isn't needed for the NBC method?

This comment has been minimized.

@Iron-Stark

Iron-Stark Aug 14, 2017

Contributor

actually mlr is the framework which uses other libraries like e1071, caret, etc to combine all the Machine Learning packages in R to a single source. If we implement Naive Bayes here for e.g. mlr uses e1071 to train the model. The methods makeClassifTask, makeLearner are all parts of mlr and the train will use e1071 to train it.

@Iron-Stark

Iron-Stark Aug 14, 2017

Contributor

actually mlr is the framework which uses other libraries like e1071, caret, etc to combine all the Machine Learning packages in R to a single source. If we implement Naive Bayes here for e.g. mlr uses e1071 to train the model. The methods makeClassifTask, makeLearner are all parts of mlr and the train will use e1071 to train it.

This comment has been minimized.

@zoq

zoq Aug 14, 2017

Member

Ah, okay, thanks for the clarification.

@zoq

zoq Aug 14, 2017

Member

Ah, okay, thanks for the clarification.

Show outdated Hide outdated methods/R/nbc.r
@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 14, 2017

Member

Let's keep the NBC script.

Member

zoq commented Aug 14, 2017

Let's keep the NBC script.

@zoq

zoq approved these changes Aug 14, 2017

@@ -48,6 +48,7 @@ fi
./nearpy_install.sh $1
if [ "$?" -ne "0" ]; then
echo "Error installing nearpy!";
exit 1;

This comment has been minimized.

@zoq

zoq Aug 14, 2017

Member

Nice catch!

@zoq

zoq Aug 14, 2017

Member

Nice catch!

@zoq zoq merged commit 2eac56d into mlpack:master Aug 14, 2017

1 check passed

Benchmarks Checks Build finished.
Details
@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 14, 2017

Member

Nice work, easy merge!

Member

zoq commented Aug 14, 2017

Nice work, easy merge!

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