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

Remove implicit pinvs of vectors #39987

Open
antoine-levitt opened this issue Mar 11, 2021 · 8 comments · May be fixed by #40758
Open

Remove implicit pinvs of vectors #39987

antoine-levitt opened this issue Mar 11, 2021 · 8 comments · May be fixed by #40758

Comments

@antoine-levitt
Copy link
Contributor

This is breaking, so probably has to wait until 2.0. At that point, can we remove /(x::Number, v::AbstractVector)? Its (marginal) utility is outmatched by its potential for confusion for new users and subtle bugs even for more experienced coders. More generally there are two and only two places where we call pinv on vectors implicitly:

(\)(a::AbstractVector, b::AbstractArray) = pinv(a) * b
/(x::Number, v::AbstractVector) = x*pinv(v)

I think we should just remove both. Pinv of a vector is just too confusing to be called implicitly. In fact, this is even more confusing because we don't allow number / matrix or matrix \ number, although pinv of a matrix makes way more sense than pinv of a vector. The fact that nobody has bothered adding this makes me think this function won't be missed.

@StefanKarpinski StefanKarpinski added linalg triage status:triage This should be discussed on a triage call and removed status:triage This should be discussed on a triage call labels May 7, 2021
@ViralBShah
Copy link
Member

ViralBShah commented May 7, 2021

Yes, it does seem like we need to wait until 2.0.

We could prepare a PR and run package evaluator to see how breaking it is. If not, we could just roll it out.

@dlfivefifty
Copy link
Contributor

One could argue that matrix \ vector for least squares is also confusing and that there should be a separate syntax when matrix is not-invertible, e.g., matrix \\ vector, when a user actually really does want least squares

@antoine-levitt
Copy link
Contributor Author

That'd be even more breaking and a lot more controversial, but I'd be in favor also.

Another possibility is to keep \ as is, but remove / entirely. It's very rarely useful as a matrix-vector op (because of the "vectors are columns" convention), and many people don't even know it exists. I've only ever used it for change of basis (eg P * D / P)

@dlfivefifty
Copy link
Contributor

I use / all the time... e.g. 1 / 2....

I like the principle of / and \ being just left and right versions of the same operation. Anything else feels arbitrary from a mathematical perspective (though I do agree anyone coming from Matlab will only ever use \ for linear algebra.)

It's the fact that either uses least squares that's the problem, particularly since it's completely inconsistent between square and rectangular matrices:

julia> zeros(1,1) \ [1]
1-element Vector{Float64}:
 Inf

julia> zeros(2,1) \ [1,1]
1-element Vector{Float64}:
 0.0

julia> pinv(zeros(1,1)) * [1]
1-element Vector{Float64}:
 0.0

Having a distinct syntax would fix this.

@antoine-levitt
Copy link
Contributor Author

antoine-levitt commented May 11, 2021

Pseudo-inverses are A^+, so possibly /+ and \+ could be made to parse as operators? Although you can certainly use least squares without being aware of pseudoinverses, so it might not be useful to reference pseudo inverses

@ViralBShah
Copy link
Member

Agree that removing the \ for least squares could be very disruptive, and is something to be considered 2.0.

@martinholters
Copy link
Member

Also ref. #28827 regarding making / and \ only do division in the narrower sense. I'd be in favor, but agree that it's obviously 2.0-stuff.

@ViralBShah
Copy link
Member

Fix for the scalar case in #44358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants