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

Classic FrankWolfe and OMP #1041

Merged
merged 16 commits into from Jul 25, 2017

Conversation

Projects
None yet
5 participants
@czdiao
Contributor

czdiao commented Jun 27, 2017

The classic FrankWolfe and OMP is working with this version. They passed simple tests. Please review it.

czdiao added some commits Jun 6, 2017

@rcurtin

Hi Chenzhe, thanks for your hard work putting this together. I took a first pass and left some comments; I hope they are helpful. There are lots of style issue but you should be able to use Jenkins to point them out and fix them. If you want me to explain that system I am happy to, just let me know.

czdiao added some commits Jul 3, 2017

@zoq

Unfortunately the style check didn't pick up all issues, so here are some more.

@stephentu

This comment has been minimized.

Show comment
Hide comment
@stephentu

stephentu Jul 12, 2017

Contributor

@rcurtin: is this mergable now? what are the outstanding issues that need to be resolved? thanks!

Contributor

stephentu commented Jul 12, 2017

@rcurtin: is this mergable now? what are the outstanding issues that need to be resolved? thanks!

@zoq

It looks like we have to wait at least for: #1050. But I think we can do that today or tomorrow.

Show outdated Hide outdated src/mlpack/core/math/lin_alg.hpp
@rcurtin

@stephentu: the call is up to you---if you have taken a look through and you are happy with the correctness of the implementation, the tests, etc., then you can post that you'll merge it in X days to allow people to take a look, then hit the merge button in X days. This one has been open for long enough that I'd say X=3 would be fine.

@czdiao: thanks for addressing my earlier comments, sorry that I did not manage to reply further. I think it's ok to require that UpdateSpan can only be used with FuncSq, as long as the documentation makes that clear.

I found another couple little style notes and other things, nothing big. If you can handle them it would be great. Otherwise I think this is good to go, I think it is a nice improvement. 👍

czdiao added some commits Jul 16, 2017

@czdiao

This comment has been minimized.

Show comment
Hide comment
@czdiao

czdiao Jul 16, 2017

Contributor

@rcurtin Thanks for pointing out the problem. I hope this works now.

Contributor

czdiao commented Jul 16, 2017

@rcurtin Thanks for pointing out the problem. I hope this works now.

@stephentu

This comment has been minimized.

Show comment
Hide comment
@stephentu

stephentu Jul 17, 2017

Contributor

@rcurtin these changes look good to me. I will merge this in 3 days!

Contributor

stephentu commented Jul 17, 2017

@rcurtin these changes look good to me. I will merge this in 3 days!

@zoq

zoq approved these changes Jul 17, 2017

Just made some minor comments, excited to see this merged.

@zoq zoq referenced this pull request Jul 21, 2017

Closed

Memory head of NTM module #1067

@stephentu stephentu merged commit 6a43191 into mlpack:master Jul 25, 2017

3 checks passed

Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment