Semi-Supervised SRM code #108
Conversation
|
@cameronphchen Could you please review this PR as well? Thank you. |
|
@TuKo Sure, I'll do a review. Do you mind fixing the failed Travis CI first? I checked the error message, but not quite sure what's the cause. |
|
@cameronphchen, I looked at the build and didn't find any actual problem related to the code. It is failing only in one version of Mac and it is fine in the other one for Mac and Linux. The error does not seem related to the tests which is actually passing (https://travis-ci.org/IntelPNI/brainiak/jobs/161482100#L303). It is probably related to some library that we are using for the testing. I will check this with @mihaic later. Please, go ahead and don't wait for this to be resolved. I will try to update during the day. Thanks. |
|
@cameronphchen Indeed, the Travis error is not related to this PR. Please proceed with the review. |
|
@cameronphchen Please remember about this review. |
|
Yes, I still remember this. Will try to finish before the Friday meeting. On Wed, Sep 28, 2016, 13:29 Mihai Capotă notifications@github.com wrote:
|
cameronphchen
left a comment
There was a problem hiding this comment.
@TuKo @mihaic Overall looks good to me. Thank you Javier for sending in this PR!!! I have commented on some minor points.
Besides, is it possible to have some kind of test that's dependent on the dataset? For example, since we are using raider as the example dataset, can we have some kind of test that makes sure the results are always consistent on the raider dataset, e.g. image classification accuracy has to be X%+- 0.1%?
brainiak/funcalign/sssrm.py
Outdated
| data to train a Multinomial Logistic Regression (MLR) classifier (with | ||
| l2 regularization) in a semi-supervised manner: | ||
| .. math:: X_i \\approx W_i S ,~for~all~i=1\dots N |
There was a problem hiding this comment.
Is there a reason we write the math like this instead of the objective function? It's a bit unclear how do the parameters affect the model. In other words, there's no gamma and "cost" in the math.
brainiak/funcalign/sssrm.py
Outdated
| gamma : float, default: 1.0 | ||
| Regularization parameter for the classifier. | ||
| cost : float, default: 0.5 |
There was a problem hiding this comment.
can we not use "cost" as the name? This name is a bit confusing.... especially this is just a parameter not really a cost. We use alpha in the paper, can we also use alpha here?
brainiak/funcalign/sssrm.py
Outdated
| bias): | ||
| """Compute the objective function of the Semi-Supervised SRM | ||
| .. math:: (1-C)*Loss_{SRM}(W_i,S;X_i) |
There was a problem hiding this comment.
same here, can we use notations that are consistent to the paper?
| raise | ||
|
|
||
|
|
||
| def sumexp_stable(data): |
There was a problem hiding this comment.
why do we need this instead of a softmax function directly? are we going to use this besides the calculation in softmax?
There was a problem hiding this comment.
We are using this for the objective function computation. This is only for logging.
|
@cameronphchen To test on the whole Raiders data we need long-running tests, which are different from the short-running tests we run now for PRs. We have an issue for documenting acceptable runtimes for the existing tests (issue #107) and an issue for long-running tests (issue #123). We can discuss those issues, but I think we should not hold this PR until they are solved; if you would like, you can create an SSSRM-specific testing issue blocked by issue #123. |
|
@mihaic Definitely, that shouldn't be blocking this PR. |
|
Jenkins, retest this please. |
|
@cameronphchen , please review the latest changes. If everything is fine, please approve the PR. Otherwise let me know and I will add changes. |
|
Jenkins, retest this please. |
tests/utils/test_utils.py
Outdated
| try: | ||
| r = concatenate_list(l, axis=0) | ||
| except: | ||
| assert True, "Could not concatenate a list" |
Unregistered function handling
Adding the Semi-Supervised SRM code, tests and examples.
In addition, I modified the raider dataset files in the examples for SRM and SS-SRM. The SRM examples (python and notebook) were corrected accordingly.