[Do not Merge] Remove implicit pinvs of vectors

[antoine-levitt]

Fix #39987

This is breaking so should not be merged yet, I'm putting it up as suggested by @ViralBShah so PkgEval can be run to see if anybody actually uses this.

[ViralBShah]

@nanosoldier runtests(ALL, vs = ":master")

[ViralBShah]

The base tests will need fixing to be in line and all green with this before we should run pkgeval. I kicked it off a bit too soon.

[antoine-levitt]

Should be good now!

[ViralBShah]

Does marking this PR as draft prevent a nanosoldier run from triggering?

[nanosoldier]

Something went wrong when running your job:

NanosoldierError: failed to run tests: AWSException: XAmzContentSHA256Mismatch -- The provided 'x-amz-content-sha256' header does not match what was computed.
HTTP.ExceptionRequest.StatusError(400, "PUT", "/julialang-reports/nanosoldier/pkgeval/by_hash/881c0ed_vs_27d3931/OptimKit.1.7.0-DEV-e81314476e.log?x-amz-acl=public-read", HTTP.Messages.Response:
"""
HTTP/1.1 400 Bad Request
x-amz-request-id: 2YY666KAG818P6K6
x-amz-id-2: 3OSuKP8bvnenAYeulnFb84IpR5FX37IFCvrhoa1KGNi8Bems//FFacUOasPaHL2o3oW8RLUnKZY=
Content-Type: application/xml
Transfer-Encoding: chunked
Date: Sun, 09 May 2021 20:54:07 GMT
Server: AmazonS3
Connection: close

<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>XAmzContentSHA256Mismatch</Code><Message>The provided 'x-amz-content-sha256' header does not match what was computed.</Message><ClientComputedContentSHA256>a46d6b98a0005193b412e8c48b9626c652c7554a7fa8cf29c09a58c4a7920cd6</ClientComputedContentSHA256><S3ComputedContentSHA256>6671bb835e70b4055bbb18e58c414af4fb9b677a8b6848fbef1bd08afaf01b26</S3ComputedContentSHA256><RequestId>2YY666KAG818P6K6</RequestId><HostId>3OSuKP8bvnenAYeulnFb84IpR5FX37IFCvrhoa1KGNi8Bems//FFacUOasPaHL2o3oW8RLUnKZY=</HostId></Error>""")

Logs and partial data can be found here
cc @maleadt

[ViralBShah]

@nanosoldier runtests(ALL, vs = ":master")

[nanosoldier]

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

[antoine-levitt]

The following are real:
DiffEqDevTools
DomainSets
Indicators
MRIReco
QuantEcon
QuantumOpticsBase
RecordedArrays

Will take a look at some point to see which of these are overzealous testing vs real usage

[antoine-levitt]

I had some preliminary look at this. DiffEqDevTools and QuantumOpticsBase look very much like bugs. Eg DiffEqDevTools has

abstols = 1. /10 .^(3:10)
reltols = 1 ./10 .^(3:10)

which is a pretty strong argument for this PR :-) Will open issues there.

RecordedArrays and DomainSets look like overzealous testing. I could not easily determine Indicators, MRIReco and QuantEcon, I'll try to investigate locally. Also this PR results in some infinite loops, we should probably just restrict to AbstractMatrix here:

/(u::AdjointAbsVec, A::Transpose{<:Any,<:AbstractMatrix}) = adjoint(conj(A.parent) \ u.parent) # technically should be adjoint(copy(adjoint(copy(A))) \ u.parent)
/(u::TransposeAbsVec, A::Adjoint{<:Any,<:AbstractMatrix}) = transpose(conj(A.parent) \ u.parent) # technically should be transpose(copy(transpose(copy(A))) \ u.parent)

[ViralBShah]

@ChrisRackauckas Note the DiffeqDevTools issue here.

[antoine-levitt]

Indicators, MRIReco (via NFFT) and QuantEcon are legitimate: they do x \ y to fit y = a*x in least squares. Under this PR x would have to be converted to a matrix before doing that. We could plausibly kill something / vector but not vector \ something. That would be inconsistent, but it would catch most errors, while keeping the full functionality of backslashes (backslashes usually imply tricky linear algebra intent, unlike forward slashes)

[antoine-levitt]

So summing up, seven failures: one overzealous testing, two using number / vector by error, and four using vector \ vector on purposes for linear regression. That supports removing number / vector at least.

[simeonschaub]

We probably should make a definite decision here. I have marked it for linalg triage now, but I'm not sure it ever convened, so we might just want to regularly triage this?

[ViralBShah]

Should probably rebase to master and rerun pkgeval.

[antoine-levitt]

Done. Let's see if I can summon?
@nanosoldier runtests(ALL, vs = ":master")
edit: nope, doesn't seem like I can, can someone with permissions do it?

[maleadt]

@nanosoldier runtests(ALL, vs = ":master")

[nanosoldier]

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

[antoine-levitt]

Nothing new here.

To sum up, there's two methods here: 1 scalar / vector 2 vector / vector, which through fallbacks provides vector \ vector.

1 is not used in any package as far as I can see, and I have trouble seeing a use for it. It's very dangerous because of the possible confusion with scalar ./ vector (which is much more used): it also produces an array and so has the potential for silent corruption (as was found in several packages in the previous pkgeval run). It is the worst offender and should ideally be removed ASAP, if possible.

Regarding 2, vector \ vector is legitimately used in packages for fitting one vector to another, which is nice and useful. vector / vector is much less useful. My guess is that this is because x \ y is overdetermined and simply defined as the scalar a that minimizes ||x-a y||, whereas x / y is undetermined and is defined as the matrix that minimizes ||x - A y|| and has minimal norm.

I think we should definitely do 1 ASAP. Regarding 2, we could leave it as is, remove only vector / vector, or also remove vector \ vector. I would be in favor of removing both of them. The rule would then be that vectors are never pseudo-inverted implicitly and have to be converted to matrices. The deprecation would be from x / y to x / hcat(y) and x \ y to hcat(x) \ y.

Removing either is technically breaking, so I'm not sure how eligible that is in the 1.x timeline. We should not remove vector \ vector before 2.0, but scalar / vector was a very odd one to begin with (eg vector \ scalar doesn't exist and it never bothered anyone), and doesn't appear to be used (other than in error)

[ViralBShah]

@StefanKarpinski you should probably take a look at this issue if you haven't already.

[ViralBShah]

Did the linear algebra triage call happen? I think @antoine-levitt's analysis is spot on. Since this PR also removes vector \ vector, doesn't it need to be updated to leave that in and just remove the scaler version?

[ViralBShah]

Also pinging @KristofferC

[andreasnoack]

Did the linear algebra triage call happen?

Not yet.

[ViralBShah]

@antoine-levitt I wonder if we should get this in. Can you rebase this? We'll run pkgeval once more and merge unless there is any further concern.

[antoine-levitt]

Thanks for pinging, I'd forgotten this. I'm assuming you meant only the scalar / vector part? I opened up a new PR, #44358, for that. I'm keeping this one, which has the more breaking vector/vector and vector\vector

[ViralBShah]

I suppose the more breaking stuff is a 2.0 thing.