Skip to content
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

Semi-Supervised SRM code #108

Merged
merged 17 commits into from Sep 30, 2016
Merged

Semi-Supervised SRM code #108

merged 17 commits into from Sep 30, 2016

Conversation

TuKo
Copy link
Contributor

@TuKo TuKo commented Sep 20, 2016

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.

@TuKo
Copy link
Contributor Author

TuKo commented Sep 20, 2016

@cameronphchen Could you please review this PR as well? Thank you.

@cameronphchen
Copy link
Contributor

@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.

@TuKo
Copy link
Contributor Author

TuKo commented Sep 21, 2016

@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.

@mihaic
Copy link
Member

mihaic commented Sep 21, 2016

@cameronphchen Indeed, the Travis error is not related to this PR. Please proceed with the review.

@mihaic
Copy link
Member

mihaic commented Sep 28, 2016

@cameronphchen Please remember about this review.

@cameronphchen
Copy link
Contributor

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 https://github.com/cameronphchen Please remember about
this review.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#108 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACqJEj81mj6IDeZw7i4S1mSS3omyjMMFks5quqQRgaJpZM4KCJUi
.

Copy link
Contributor

@cameronphchen cameronphchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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%?

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

gamma : float, default: 1.0
Regularization parameter for the classifier.

cost : float, default: 0.5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

bias):
"""Compute the objective function of the Semi-Supervised SRM

.. math:: (1-C)*Loss_{SRM}(W_i,S;X_i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, can we use notations that are consistent to the paper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -105,6 +105,71 @@ def fast_inv(a):
raise


def sumexp_stable(data):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this instead of a softmax function directly? are we going to use this besides the calculation in softmax?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using this for the objective function computation. This is only for logging.

@mihaic
Copy link
Member

mihaic commented Sep 30, 2016

@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.

@cameronphchen
Copy link
Contributor

@mihaic Definitely, that shouldn't be blocking this PR.

@TuKo
Copy link
Contributor Author

TuKo commented Sep 30, 2016

Jenkins, retest this please.

@TuKo
Copy link
Contributor Author

TuKo commented Sep 30, 2016

@cameronphchen , please review the latest changes. If everything is fine, please approve the PR. Otherwise let me know and I will add changes.
(Linux testing is failing because there is some issue with the system)

@mihaic
Copy link
Member

mihaic commented Sep 30, 2016

Jenkins, retest this please.

Copy link
Contributor

@cameronphchen cameronphchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

try:
r = concatenate_list(l, axis=0)
except:
assert True, "Could not concatenate a list"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use pytest.raises, please.

@mihaic mihaic merged commit cbb3b98 into brainiak:master Sep 30, 2016
@TuKo TuKo deleted the semi3 branch September 30, 2016 20:37
danielsuo pushed a commit that referenced this pull request Nov 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants