You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@madlib.apache.org by iyerr3 <gi...@git.apache.org> on 2018/03/22 16:59:33 UTC
[GitHub] madlib pull request #247: SVM: Revert minibatch-related work
GitHub user iyerr3 opened a pull request:
https://github.com/apache/madlib/pull/247
SVM: Revert minibatch-related work
This commit is a partial revert of a8bbe08.
Minibatch was not found to be useful for SVM and broken due to recent
changes. We're disabling and reverting that work, with the task of
revisiting this again when the minibatch interface is stable.
There were some changes in a8bbe08 that were unrelated to minibatch and
useful in general context. These changes have not been reverted.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/madlib/madlib feature/revert_svm_minibatch
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/madlib/pull/247.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #247
----
commit cccf971a74c9d88466895b90d02089b42157f9c0
Author: Rahul Iyer <ri...@...>
Date: 2018-03-22T02:28:47Z
SVM: Revert minibatch-related work
This commit is a partial revert of a8bbe08.
Minibatch was not found to be useful for SVM and broken due to recent
changes. We're disabling and reverting that work, with the task of
revisiting this again when the minibatch interface is stable.
There were some changes in a8bbe08 that were unrelated to minibatch and
useful in general context. These changes have not been reverted.
----
---
[GitHub] madlib issue #247: SVM: Revert minibatch-related work
Posted by kaknikhil <gi...@git.apache.org>.
Github user kaknikhil commented on the issue:
https://github.com/apache/madlib/pull/247
@iyerr3
I compared this PR with the original commit and it looks good. Just a couple of minor questions
1. This change to model.hpp was not reverted. It looks like it's not related to minibatch but just wanted to confirm it with you
```
- for (k = 1; k <= N; k ++) {
- u.push_back(Eigen::Map<Matrix >(
- const_cast<double*>(data + sizeOfU),
- n[k-1] + 1, n[k]));
- sizeOfU += (n[k-1] + 1) * (n[k]);
+ for (k = 0; k < N; k ++) {
+ // u.push_back(Eigen::Map<Matrix >(
+ // const_cast<double*>(data + sizeOfU),
+ // n[k] + 1, n[k+1]));
+ u.push_back(MutableMappedMatrix());
+ u[k].rebind(const_cast<double *>(data + sizeOfU), n[k] + 1, n[k+1]);
+ sizeOfU += (n[k] + 1) * (n[k+1]);
```
2. A function named `initialize` was added to model.hpp but I didn't see it getting used anywhere. Should this be reverted as well ?
I also cherry picked this revert commit on top of the mlp minibatch branch `mlp-minibatch-with-preprocessed-data-rebased` and successfully ran install check for svm and mlp with greenplum.
---
[GitHub] madlib issue #247: SVM: Revert minibatch-related work
Posted by iyerr3 <gi...@git.apache.org>.
Github user iyerr3 commented on the issue:
https://github.com/apache/madlib/pull/247
@kaknikhil
Yes, both those changes are related to MLP and ideally should have been in a different commit. We can still make it that way by reverting them here and then reintroducing these changes.
The `1.` is for not copying the coefficients every time we rebind and `2.` is for moving the coefficient initialization from python to c++, since the model is a c++ variable. They're both optimizations not necessarily required immediately.
My suggestion is that we keep the changes here and avoid a redundant commit. We've another task of fixing MLP's initialization (jira pending) - as part of that ticket we can also utilize the `initialize` function. What do you think?
---
[GitHub] madlib issue #247: SVM: Revert minibatch-related work
Posted by kaknikhil <gi...@git.apache.org>.
Github user kaknikhil commented on the issue:
https://github.com/apache/madlib/pull/247
@iyerr3
Thanks for the detailed explanation. I agree, we should avoid a redundant commit and take care of this in a future commit.
---
[GitHub] madlib pull request #247: SVM: Revert minibatch-related work
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/madlib/pull/247
---
[GitHub] madlib issue #247: SVM: Revert minibatch-related work
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit commented on the issue:
https://github.com/apache/madlib/pull/247
Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/madlib-pr-build/396/
---