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.

Sign up for GitHub

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

Jump to bottom

Add random_state #147

Merged
merged 13 commits into from
Mar 14, 2016
Merged

Add random_state #147

merged 13 commits into from
Mar 14, 2016

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Mar 2, 2016

Fixes #139.

So far, MVARICA supports the random_state argument. I'll add the other functions soon. In the meantime, please let me know if my changes are OK @mbillingr.

@cbrnr cbrnr changed the title Add random_state to MVARICA Add random_state to OOAPI Mar 2, 2016
@cbrnr
Copy link
Contributor Author

cbrnr commented Mar 2, 2016

I don't understand the coverage report:

  1. How come the report is shown before the Travis build for Python 3.5 is finished? I thought this build triggers Coveralls?
  2. Why did coverage increase? What is the reference/baseline? The previous commit? The current master?

@cbrnr
Copy link
Contributor Author

cbrnr commented Mar 4, 2016

@mbillingr can you take a look at cbrnr@c83eb5b - I've disabled two tests in this function because I have no idea what they do. Also the last line in the test raised an error related to the eigenvalue decomposition (see Travis log in the previous commit) - I think not only is the input data linearly dependent(?), but there is also no assert statement to catch any errors.

@cbrnr
Copy link
Contributor Author

cbrnr commented Mar 4, 2016

Either I'm completely blind or there really is a problem with conda. Since I've tried long enough to resolve this, I reported the issue at ContinuumIO/anaconda-issues#683.

@cbrnr cbrnr changed the title Add random_state to OOAPI Add random_state Mar 4, 2016
@cbrnr cbrnr force-pushed the random_state branch 3 times, most recently from 921c8ee to 3188160 Compare March 7, 2016 09:36
@cbrnr
Copy link
Contributor Author

cbrnr commented Mar 7, 2016

I guess that's all - do you see anything I forgot?

@cbrnr
Copy link
Contributor Author

cbrnr commented Mar 7, 2016

Furthermore, what's up with Travis?

mbillingr
mbillingr reviewed Mar 8, 2016
View reviewed changes
scot/csp.py
@@ -59,7 +59,7 @@ def csp(x, cl, numcomp=None):
sigma2 += np.cov(x2[t, :, :]) / x2.shape[0]
sigma2 /= sigma2.trace()

e, w = eig(sigma1, sigma1 + sigma2, overwrite_a=True, overwrite_b=True, check_finite=False)
e, w = eigh(sigma1, sigma1 + sigma2, overwrite_a=True, overwrite_b=True, check_finite=False)
Copy link
Member

Choose a reason for hiding this comment

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

How is this change related to the random state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not :-). But when I stumbled over it, I thought it had to be changed.

Copy link
Member

Choose a reason for hiding this comment

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

Hehe, ok, but now we have to pollute this PR by discussing why this is necessary.

Why is it necessary? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we're dealing with Hermitian matrices. If our matrices are not Hermitian, this indicates a problem with our covariance matrices (e.g. due to a lack of data and/or no regularization). In such cases, eigh fails and tells us that something is wrong. eig on the other hand silently produces complex eigenvalues, which we usually ignored. For example, there was a problem with one test case, which generated a sine as input signal.

@mbillingr
Copy link
Member

What about travis?
There are some changes in this PR that I think do not quite fit the topic. Moving functions between utils and datatools is a good idea, but not really related to the random state?

@cbrnr
Copy link
Contributor Author

cbrnr commented Mar 8, 2016

What about travis?

It's working now. I guess it was related to a conda update, which rendered our numpy version non-functional. Upgrading numpy to 1.8.2 solved the problem.

However, I don't understand why Travis doesn't trigger Coveralls anymore. Any ideas?

There are some changes in this PR that I think do not quite fit the topic. Moving functions between utils and datatools is a good idea, but not really related to the random state?

Yes, it was related. Since I added check_random_state to utils and acm required atleast_3d from datatools, this resulted in a cyclic import. Moving acm solved the issue, but of course I could have also moved the import to the bottom of the file (which is ugly).

@mbillingr
Copy link
Member

However, I don't understand why Travis doesn't trigger Coveralls anymore. Any ideas?

The builds show up on the coveralls page, so I guess the problem is related to coveralls, not to travis. Otherwise I have no idea, yet.

cyclic import.

I understand.

@mbillingr
Copy link
Member

Btw,

Why did coverage increase? What is the reference/baseline?

The baseline is the current master. At the bottom of the report (coveralls page) it links to the baseline commit.

@mbillingr mbillingr added this to the 0.3 Release milestone Mar 9, 2016
@cbrnr
Copy link
Contributor Author

cbrnr commented Mar 10, 2016

@mbillingr can you check if we've caught all randomness? And if so, please feel free to merge.

@mbillingr
Copy link
Member

Can you rebase?

@cbrnr
Copy link
Contributor Author

cbrnr commented Mar 14, 2016

Yup. Done.

mbillingr added a commit that referenced this pull request Mar 14, 2016
@mbillingr
Merge pull request #147 from cbrnr/random_state
0037df5 
Add random_state
@mbillingr mbillingr merged commit 0037df5 into scot-dev:master Mar 14, 2016
@cbrnr cbrnr deleted the random_state branch March 14, 2016 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.3 Release
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@cbrnr @mbillingr