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

Add mean and covariance to the MvNormal variations #120

Merged
merged 7 commits into from
Jan 10, 2021
Merged

Add mean and covariance to the MvNormal variations #120

merged 7 commits into from
Jan 10, 2021

Conversation

jlperla
Copy link
Contributor

@jlperla jlperla commented Oct 13, 2020

No description provided.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Thanks! I left some comments and suggestions.

The latest version of Requires breaks the tests. It would be good to merge the fixes in #119 first, to make sure that tests actually pass in this PR.

Project.toml Outdated Show resolved Hide resolved
src/DistributionsAD.jl Outdated Show resolved Hide resolved
src/multivariate.jl Outdated Show resolved Hide resolved
@jlperla
Copy link
Contributor Author

jlperla commented Oct 13, 2020

Are you able to take this over and fix it to your hearts content? These things are gettingb above my git paygrade

test/others.jl Outdated
@test cov(d1) ≈ cov(d2) rtol=1e-6

A = randn(10)
C = A * A' + I
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have testing for a larger category of matrices.

@devmotion
Copy link
Member

Are you able to take this over and fix it to your hearts content? These things are gettingb above my git paygrade

I applied the suggestions (one can just commit them in Github's webinterface, next to the comment with the suggestion). There's some minor issue with ReverseDiff and Skellam in the linked PR (which contains the fixes for the CI). I haven't merged it yet since I am waiting for @mohamed82008's suggestions on how to fix this problem. Let's see if it will be fixed within the next few days - otherwise I'll extract the CI fixes and merge them in a separate PR.

@jlperla
Copy link
Contributor Author

jlperla commented Oct 13, 2020

Thanks so much!

@nmheim
Copy link
Collaborator

nmheim commented Oct 20, 2020

while we are at it, would it be ok to also add the var methods? :)

@devmotion
Copy link
Member

It seems there was some problem with merging master.

@jlperla
Copy link
Contributor Author

jlperla commented Jan 10, 2021

@devmotion I tried to rebase to see if the CI is working now, but my gitfoo is weak. This might be something you can more efficiently do. Hope that I didn't mess things up. If you completely drop this PR and just redo your change to a new one I wouldn't be offended. Sorry if I made things worse.

@devmotion
Copy link
Member

No worries, I'll have a look and fix the git issues. I'm sorry, somehow I forgot this PR.

@jlperla
Copy link
Contributor Author

jlperla commented Jan 10, 2021

No apologies at all, you are doing so much. Sorry I couldn't be more useful.

If you do decide to throw this out and restart (and it is a trivial addition), maybe add in the variance that @nmheim suggested?

@devmotion
Copy link
Member

maybe add in the variance that @nmheim suggested?

I agree, we should add it regardless of how the git issues are resolved 🙂

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Looks good to me now, but since I got involved and added some commits someone else should approve the PR.

Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

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

Am happy!

@devmotion devmotion merged commit 6976dfb into TuringLang:master Jan 10, 2021
@devmotion
Copy link
Member

Thanks a lot @jlperla!

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.

5 participants