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 Matlab implementations #89

Merged
merged 50 commits into from Jul 17, 2017

Conversation

Projects
None yet
3 participants
@Iron-Stark
Contributor

Iron-Stark commented Jul 12, 2017

@zoq @rcurtin
I will keep on adding MATLAB implementations to this PR.

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 12, 2017

Contributor

@mlpack-jenkins test this

Contributor

Iron-Stark commented Jul 12, 2017

@mlpack-jenkins test this

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 12, 2017

Contributor

@zoq @rcurtin
I will add the test for SVC once PR #88 is merged because I have written a test file there and once it is merged I can directly add to that.

Contributor

Iron-Stark commented Jul 12, 2017

@zoq @rcurtin
I will add the test for SVC once PR #88 is merged because I have written a test file there and once it is merged I can directly add to that.

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 12, 2017

Contributor

@zoq @rcurtin
I will add the test file for knc once the shogun implementations are merged and also other edits are needed for KNC (making k necessary) for which I will open another PR after the ones I am working on are merged.

Contributor

Iron-Stark commented Jul 12, 2017

@zoq @rcurtin
I will add the test file for knc once the shogun implementations are merged and also other edits are needed for KNC (making k necessary) for which I will open another PR after the ones I am working on are merged.

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 12, 2017

Contributor

@rcurtin @zoq

I was thinking that we should work on all the existing PR's and merge them first (including this one) because I am facing certain dependency issues for writing tests. Actually the problem is that I updated my master branch itself in the shogun PR so facing certain issues while creating new branches. Please let me know what you think.

Contributor

Iron-Stark commented Jul 12, 2017

@rcurtin @zoq

I was thinking that we should work on all the existing PR's and merge them first (including this one) because I am facing certain dependency issues for writing tests. Actually the problem is that I updated my master branch itself in the shogun PR so facing certain issues while creating new branches. Please let me know what you think.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jul 12, 2017

Member

Sounds fine for me.

Member

zoq commented Jul 12, 2017

Sounds fine for me.

@rcurtin

Were you planning on adding or updating any more implementations? Or is this ready to go?

If it is ready, I left a few comments, if you can address them I think this is ready to merge. If you're still adding more, let me know when it is ready for a second review. :)

@@ -1034,6 +1060,44 @@ methods:
format: [csv, txt, xml]
datasets:
- files: [ ['datasets/artificial_2DSignal.csv', 'datasets/artificial_2DSignal_hmm.xml'] ]
DTC:
run: ['metric']
script: methods/matlab/dtc.py

This comment has been minimized.

@rcurtin

rcurtin Jul 13, 2017

Member

It would be worth pointing out in a comment or somewhere else that this requires the statistics toolbox---not everyone might have that installed. (We do, so it's no problem for us, but it would be good to let others know in case they are trying to use it.) I think this applies to all the methods you've added here actually, not just the decision tree classifier.

@rcurtin

rcurtin Jul 13, 2017

Member

It would be worth pointing out in a comment or somewhere else that this requires the statistics toolbox---not everyone might have that installed. (We do, so it's no problem for us, but it would be good to let others know in case they are trying to use it.) I think this applies to all the methods you've added here actually, not just the decision tree classifier.

Show outdated Hide outdated methods/matlab/DTC.m
Show outdated Hide outdated methods/matlab/SVC.m
Log.Info("Perform KNC.", self.verbose)
self.opts = {}
if "k" in options:
self.opts["n_neighbors"] = int(options.pop("k"))

This comment has been minimized.

@rcurtin

rcurtin Jul 13, 2017

Member

I would suggest we make this required, but I think we've already discussed that and you said it would be done in a different PR; is that correct?

@rcurtin

rcurtin Jul 13, 2017

Member

I would suggest we make this required, but I think we've already discussed that and you said it would be done in a different PR; is that correct?

This comment has been minimized.

@Iron-Stark

Iron-Stark Jul 14, 2017

Contributor

@rcurtin

Yes once all this is merged I update all the KNC's together in another PR.

@Iron-Stark

Iron-Stark Jul 14, 2017

Contributor

@rcurtin

Yes once all this is merged I update all the KNC's together in another PR.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jul 14, 2017

Member

@Iron-Stark Great, thanks! I think we should open a new PR for the changes.

Member

zoq commented Jul 14, 2017

@Iron-Stark Great, thanks! I think we should open a new PR for the changes.

@rcurtin

Everything looks good to me. :)

@zoq

Looks good to me, just some minor issues, also can you write a test for the KNC and SVC script?

Show outdated Hide outdated methods/matlab/dtc.py
Show outdated Hide outdated methods/matlab/dtc.py
Show outdated Hide outdated methods/matlab/knc.py
Show outdated Hide outdated methods/matlab/knc.py
Show outdated Hide outdated methods/matlab/svc.py
@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 14, 2017

Contributor

@zoq

Regarding test for KNC I am working on changing implementations of all KNC scripts to make specifying the parameter k mandatory. I will add the test in that PR itself. I have added one for SVC here.

Contributor

Iron-Stark commented Jul 14, 2017

@zoq

Regarding test for KNC I am working on changing implementations of all KNC scripts to make specifying the parameter k mandatory. I will add the test in that PR itself. I have added one for SVC here.

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 15, 2017

Contributor

@zoq @rcurtin

Looks like we need to abort the current build.

Contributor

Iron-Stark commented Jul 15, 2017

@zoq @rcurtin

Looks like we need to abort the current build.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jul 15, 2017

Member

@Iron-Stark Right, let me do this.

Member

zoq commented Jul 15, 2017

@Iron-Stark Right, let me do this.

Iron-Stark added some commits Jul 15, 2017

Remove shogun test
The shogun SVM implementation needs to be updated. Will do that in another PR. Keeping this one clean for only MATLAB implementations.
@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 15, 2017

Contributor

@zoq @rcurtin

The shogun SVM implementation needs updation. Opening up a separate PR for the same.

Contributor

Iron-Stark commented Jul 15, 2017

@zoq @rcurtin

The shogun SVM implementation needs updation. Opening up a separate PR for the same.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jul 17, 2017

Member

I tested the MulticlassLibSVM class, which should handle multi class problems, but I run into some strange errors:

methods/shogun/svm.py:98: RuntimeWarning: [WARN] In file benchmark/libraries/shogun/src/shogun/multiclass/MulticlassOneVsOneStrategy.cpp line 34: MulticlassOneVsOneStrategy::CMulticlassOneVsOneStrategy(): register parameters!

  svm = MulticlassLibSVM(self.C, self.kernel, labels)

I wonder if we have to install libsvm as the dependency. Anyway let's open up an new PR to solve the issue.

Member

zoq commented Jul 17, 2017

I tested the MulticlassLibSVM class, which should handle multi class problems, but I run into some strange errors:

methods/shogun/svm.py:98: RuntimeWarning: [WARN] In file benchmark/libraries/shogun/src/shogun/multiclass/MulticlassOneVsOneStrategy.cpp line 34: MulticlassOneVsOneStrategy::CMulticlassOneVsOneStrategy(): register parameters!

  svm = MulticlassLibSVM(self.C, self.kernel, labels)

I wonder if we have to install libsvm as the dependency. Anyway let's open up an new PR to solve the issue.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Jul 17, 2017

Member

If the libsvm issue will be solved in another PR, should we go ahead and merge this one now then?

Member

rcurtin commented Jul 17, 2017

If the libsvm issue will be solved in another PR, should we go ahead and merge this one now then?

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 17, 2017

Contributor

@rcurtin

If this is ready to be merged then we can.

Contributor

Iron-Stark commented Jul 17, 2017

@rcurtin

If this is ready to be merged then we can.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jul 17, 2017

Member

@Iron-Stark is there anything you like to add? Looks ready for me 👍

Member

zoq commented Jul 17, 2017

@Iron-Stark is there anything you like to add? Looks ready for me 👍

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 17, 2017

Contributor

@zoq

No I wont be adding anything to this PR. If its look ready we can merge it.

Contributor

Iron-Stark commented Jul 17, 2017

@zoq

No I wont be adding anything to this PR. If its look ready we can merge it.

@zoq

zoq approved these changes Jul 17, 2017

Looks good to me.

@rcurtin rcurtin merged commit ad0f398 into mlpack:master Jul 17, 2017

1 check passed

Benchmarks Checks Build finished.
Details
@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jul 17, 2017

Member

Thanks for another great contribution!

Member

zoq commented Jul 17, 2017

Thanks for another great contribution!

Iron-Stark added a commit to Iron-Stark/benchmarks that referenced this pull request Jul 19, 2017

Merge pull request #89 from Iron-Stark/patch-20
Updating Matlab implementations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment