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

Making sure user has passed the test file and avoid failing in case it is not passed. #91

Merged
merged 48 commits into from Jul 27, 2017

Conversation

Projects
None yet
3 participants
@Iron-Stark
Contributor

Iron-Stark commented Jul 12, 2017

@zoq @rcurtin

I have updated the files from the previous merged PR's. Please Review.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Jul 13, 2017

Member

The changes look good to me, but should we also do these for the other scikit methods? i.e. elastic_net.py, perceptron.py, svm.py, etc.? In those methods and some others it looks like predict() ends up getting called twice because the results are not saved to self.predictions the first time.

Member

rcurtin commented Jul 13, 2017

The changes look good to me, but should we also do these for the other scikit methods? i.e. elastic_net.py, perceptron.py, svm.py, etc.? In those methods and some others it looks like predict() ends up getting called twice because the results are not saved to self.predictions the first time.

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 14, 2017

Contributor

@rcurtin

Sure I will do this and add it to this PR itself.

Contributor

Iron-Stark commented Jul 14, 2017

@rcurtin

Sure I will do this and add it to this PR itself.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Jul 14, 2017

Member

I think maybe you aren't done with this yet, but I think that linear_ridge_regression.py and logistic_regression.py still need to be done.

Member

rcurtin commented Jul 14, 2017

I think maybe you aren't done with this yet, but I think that linear_ridge_regression.py and logistic_regression.py still need to be done.

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 14, 2017

Contributor

@rcurtin

Yes they need to be done. For reasons yet unknown the linear ridge regression tests fail. :( trying to figure out the reason.

Contributor

Iron-Stark commented Jul 14, 2017

@rcurtin

Yes they need to be done. For reasons yet unknown the linear ridge regression tests fail. :( trying to figure out the reason.

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 14, 2017

Contributor

@rcurtin

Logistic Regression is already done. That was updated in an older merged PR itself. (PR #83)

Contributor

Iron-Stark commented Jul 14, 2017

@rcurtin

Logistic Regression is already done. That was updated in an older merged PR itself. (PR #83)

@Iron-Stark

This comment has been minimized.

Show comment
Hide comment
@Iron-Stark

Iron-Stark Jul 14, 2017

Contributor

@rcurtin @zoq

All the implementations have been updated. Please review.

Contributor

Iron-Stark commented Jul 14, 2017

@rcurtin @zoq

All the implementations have been updated. Please review.

@rcurtin

Looks good to me other than the comments about regression.

Show outdated Hide outdated methods/scikit/linear_regression.py
@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Jul 20, 2017

Member

It looks like logistic_regression.py isn't updated with the q.put([time]) strategy.

Member

rcurtin commented Jul 20, 2017

It looks like logistic_regression.py isn't updated with the q.put([time]) strategy.

Show outdated Hide outdated methods/scikit/adaboost.py
Show outdated Hide outdated methods/scikit/adaboost.py
@rcurtin

LGTM. :)

@zoq

zoq approved these changes Jul 27, 2017

Looks good to me.

@zoq zoq merged commit 58bcaca into mlpack:master Jul 27, 2017

1 check passed

Benchmarks Checks Build finished.
Details
@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jul 27, 2017

Member

Thanks for the fix.

Member

zoq commented Jul 27, 2017

Thanks for the fix.

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