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

Matmul warning #1240

Merged
merged 4 commits into from Feb 16, 2021
Merged

Matmul warning #1240

merged 4 commits into from Feb 16, 2021

Conversation

rileyjmurray
Copy link
Collaborator

This addresses #1238 and is intended to supersede #1239. It removes side-effects of our existing warnings for accessing matmul through * and introduces a new MatmulWarning class for that purpose.

This PR also updates the web doc source to mention the importance of cvxpy 1.1.10 for the numpy installation issue.

@akshayka
Copy link
Collaborator

I think raising a MatmulWarning instead of a DeprecationWarning is bit odd. Production could might have policies that promote DeprecationWarning's to errors. A custom MatmulWarning will go undetected, unless a human sees it.

Why not raise a DeprecationWarning, which is a very standard type of warning? Humans will understand it, and so will codebases. Its semantics are very clear.

@akshayka
Copy link
Collaborator

akshayka commented Feb 14, 2021

That said I don't feel strongly, so I won't block this! I'll leave it to @SteveDiamond to make the call

@rileyjmurray
Copy link
Collaborator Author

We changed from a DeprecationWarning to a UserWarning a while back, because DeprecationWarnings are not as visible to users: https://github.com/cvxgrp/cvxpy/issues/980 and https://github.com/cvxgrp/cvxpy/pull/981.

I see your point about the semantics of DeprecationWarning though. One option would be to raise a DeprecationWarning in addition to something a user is more likely to see. I'd be okay with removing the custom MatmulWarning if we raised both a DeprecationWarning and UserWarning. I think both are actually justified, as the DeprecationWarning says "you should change this code and may have to change code elsewhere to be compliant in the future", while a UserWarning says "this might not have done what you thought it was going to do."

@akshayka what do you think about the two-warning option?

@akshayka
Copy link
Collaborator

@rileyjmurray , that sounds good to me!

@SteveDiamond
Copy link
Collaborator

I'm also fine with the two warnings.

@rileyjmurray
Copy link
Collaborator Author

Great! I'll change it to two warnings and merge it in later today.

@rileyjmurray rileyjmurray merged commit 636ec4a into cvxpy:master Feb 16, 2021
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