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

Updating Weka implementations. #95

Merged
merged 11 commits into from Jul 29, 2017

Conversation

Projects
None yet
3 participants
@Iron-Stark
Contributor

Iron-Stark commented Jul 22, 2017

No description provided.

Show outdated Hide outdated methods/weka/decision_stump.py
self.dataset = dataset
self.path = path
self.timeout = timeout

This comment has been minimized.

@zoq

zoq Jul 23, 2017

Member

I think we should add a destructor here to clean up, e.g. remove the weka_predicted.csv file.

@zoq

zoq Jul 23, 2017

Member

I think we should add a destructor here to clean up, e.g. remove the weka_predicted.csv file.

Show outdated Hide outdated methods/weka/src/DTC.java
@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 23, 2017

Contributor

@mlpack-jenkins test this

Contributor

Iron-Stark commented Jul 23, 2017

@mlpack-jenkins test this

@rcurtin

Thanks for updating all the Weka scripts. :) I left some comments, could you address them please?

Show outdated Hide outdated config.yaml
Show outdated Hide outdated methods/weka/decision_stump.py
Show outdated Hide outdated methods/weka/dtc.py
Show outdated Hide outdated methods/weka/logistic_regression.py
Show outdated Hide outdated methods/weka/perceptron.py
Show outdated Hide outdated methods/weka/random_forest.py
Show outdated Hide outdated methods/weka/src/DECISIONSTUMP.java
Show outdated Hide outdated methods/weka/src/NBC.java
nm.setOptions(options);
nm.setInputFormat(trainData);
trainData = Filter.useFilter(trainData, nm);

This comment has been minimized.

@rcurtin

rcurtin Jul 25, 2017

Member

I see that you've removed NBC from the list of methods to be benchmarked, and I also see a lot of modifications here. Can you explain why the changes were needed please?

@rcurtin

rcurtin Jul 25, 2017

Member

I see that you've removed NBC from the list of methods to be benchmarked, and I also see a lot of modifications here. Can you explain why the changes were needed please?

This comment has been minimized.

@Iron-Stark

Iron-Stark Jul 25, 2017

Contributor

I haven't removed NBC I have just placed it after LogisticRegression block. The previous implementation was not giving the correct weka_predicted file as an output. Me and @zoq had a discussion regarding this

ironstark: Turns out, weka expects that the unlabeled data set (test set) has a class, for me, this makes sense if someone likes to use the weka Evaluation class e.g. to get the MSE.

5:13 AM ironstark: However for someone just interested in the prediction ... Anyway let's add a pseudo class if there is no class, here is the updated code: https://gist.github.com/zoq/e387fd24b117890a141ebe7cff9c2abb the interesting part is line 52-61.

I used this code as a base to implement the other methods and then weka_predicted being generated was coming out correct.

@Iron-Stark

Iron-Stark Jul 25, 2017

Contributor

I haven't removed NBC I have just placed it after LogisticRegression block. The previous implementation was not giving the correct weka_predicted file as an output. Me and @zoq had a discussion regarding this

ironstark: Turns out, weka expects that the unlabeled data set (test set) has a class, for me, this makes sense if someone likes to use the weka Evaluation class e.g. to get the MSE.

5:13 AM ironstark: However for someone just interested in the prediction ... Anyway let's add a pseudo class if there is no class, here is the updated code: https://gist.github.com/zoq/e387fd24b117890a141ebe7cff9c2abb the interesting part is line 52-61.

I used this code as a base to implement the other methods and then weka_predicted being generated was coming out correct.

This comment has been minimized.

@rcurtin

rcurtin Jul 29, 2017

Member

Ah, sorry about that, I missed that you had just moved the NBC block. Thanks for the clarification.

@rcurtin

rcurtin Jul 29, 2017

Member

Ah, sorry about that, I missed that you had just moved the NBC block. Thanks for the clarification.

Show outdated Hide outdated methods/weka/src/RANDOMFOREST.java
Show outdated Hide outdated methods/weka/src/PERCEPTRON.java
Show outdated Hide outdated methods/weka/src/PERCEPTRON.java
Show outdated Hide outdated methods/weka/src/DTC.java
Show outdated Hide outdated methods/weka/random_forest.py
@zoq

zoq approved these changes Jul 28, 2017

Looks good for me!

@rcurtin

Looks good, thanks!

@rcurtin rcurtin merged commit 09ed4ee into mlpack:master Jul 29, 2017

1 check passed

Benchmarks Checks Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment