-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
MAINT: Disallow complex covariances in multivariate_normal #14733
Conversation
Looks good, except I would remove the word "yet" since there is no promise that this function will ever support complex gaussian (and IMO it should not, a complex gaussian should have its own method, but this is another discussion). |
Of course, needs a test to make sure that it is triggered. |
Let's have the discussion somewhere, because it's the difference between a |
f750c52
to
e88ca0b
Compare
This commit disallows complex covariances in multivariate_normal as passing them can silently lead to incorrect results.
e88ca0b
to
3574add
Compare
I have no clue why this is failing. It fails locally too. |
seems like the line added to the tests gives this warning: |
if np.issubdtype(cov.dtype, np.complexfloating): | ||
raise NotImplementedError("Complex gaussians are not supported.") |
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.
This only checks the covariance and not the mean - is that deliberate? If so, I think the message could be clearer
Worth being aware of these lines: |
The check is above that, and should raise an exception before the warning ever happens. |
Needs rebase. |
This looks like a pretty straightforward fix; can we revive it and get it merged? I agree with @bashtage's comment--there is no plan to make this method accept complex inputs--so the error should be a
Also @eric-wieser's comment should be followed up with a change that also raises an exception if |
Somewhat related, there was a PR to introduce a complex multivariate implementation. I don't recall what the replacement for the ordinary covariance matrix was, but apparently there is a standard approach somewhere out there in the numerical world. |
@WarrenWeckesser Feel free to take this over. |
This PR was continued in #21779, which has been merged. |
This commit disallows complex covariances in multivariate_normal
as passing them can silently lead to incorrect results.