-
Notifications
You must be signed in to change notification settings - Fork 431
Add singular branch of the Wishart #1097
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
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
34c9a36
add ispossemdef to utils
johnczito e79248a
add singular branch of wishart
johnczito 2a5c035
test singular wishart
johnczito 90bf428
update wishart test util
johnczito 0eb7a31
unit test tweak
johnczito 7fa2dd5
comment
johnczito 8e658a1
add another error
johnczito 68b0aa1
fix meanlogdet
johnczito e715ac3
use rng
johnczito 5b345fd
use count in ispossemdef
johnczito 05e7eb2
improve docstring
johnczito 41588ac
improve docstring
johnczito c405a42
return mean in all cases
johnczito 3b8f47a
return var and cov in all cases
johnczito 38f568d
test var and cov for singular
johnczito 636c695
check positivity first thing
johnczito File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a warning for expected functionality is not so nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, and I'm happy to get rid of this.
I guess I had in mind that this functionality is more unexpected for most users, so I wanted to give them a heads up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to remove the warning? I agree that it's probably not good to warn the users if Distributions supports this functionality, maybe it would be better to just mention it clearly in
show(::Wishart)? Additionally, it is inconsistent with the behaviour ofNormal(4.0, 0.0)and other special cases. An additional benefit of not printing warnings would be that the constructor would be compatible with Zygote, as noted in TuringLang/DistributionsAD.jl#84 (although probably that is not a good argument since Distributions does not try to support Zygote in the first place).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, great idea. Will do that soon. The warning was really intended more in the spirit of a deprecation: a temporary heads up about functionality people might not be expecting.
Agreed, but I am in favor of thinking through reasonable changes that
Distributionscould make that would allowTuringto simply add methods toDistributions.Wishartand so on instead of reimplementing it. I'll maybe open an issue about that someplace.