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

replace ambiguous scale! methods #24276

Closed
Sacha0 opened this issue Oct 22, 2017 · 7 comments
Closed

replace ambiguous scale! methods #24276

Sacha0 opened this issue Oct 22, 2017 · 7 comments
Labels
domain:arrays [a, r, r, a, y, s] domain:linear algebra Linear algebra stdlib Julia's standard library
Milestone

Comments

@Sacha0
Copy link
Member

Sacha0 commented Oct 22, 2017

Discussion in #16772 revealed some dubious scale! methods. For example (#16772 (comment)), determining what either of

scale!(A::AbstractArray{T<:Any,2}, b::AbstractArray{T<:Any,1}) at linalg/generic.jl:477
scale!(b::AbstractArray{T<:Any,1}, A::AbstractArray{T<:Any,2}) at linalg/generic.jl:478

does requires reading the associated code. And as @Keno suggests, expressing these operations as e.g. A*Diagonal(B) might be better. Best!

@Sacha0 Sacha0 added domain:arrays [a, r, r, a, y, s] status:triage This should be discussed on a triage call labels Oct 22, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 22, 2017

Moreover we should consider whether scale! should exist at all: Two-argument scale! methods are just in-place multiplication, and (the undocumented) three-argument scale! methods are just out-of-place multiplication. If we address #16772 (comment), all scale! methods could then simply disappear into in-place and out-of-place multiplication. Best!

@Sacha0 Sacha0 added the domain:linear algebra Linear algebra label Oct 22, 2017
@andreasnoack
Copy link
Member

The fundamental question here is exactly the same as in #16772 so let's keep the general discussion in that issue. Whatever we decide and as you point out, it makes sense to deprecate these in favor of mutating multiplication. The methods are already defined in

A_mul_B!(A::Diagonal,B::AbstractMatrix) = scale!(A.diag,B)
At_mul_B!(A::Diagonal,B::AbstractMatrix) = scale!(A.diag,B)
Ac_mul_B!(A::Diagonal,B::AbstractMatrix) = scale!(conj(A.diag),B)
A_mul_B!(A::AbstractMatrix,B::Diagonal) = scale!(A,B.diag)
A_mul_Bt!(A::AbstractMatrix,B::Diagonal) = scale!(A,B.diag)
A_mul_Bc!(A::AbstractMatrix,B::Diagonal) = scale!(A,conj(B.diag))
.

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 22, 2017

The fundamental question here is exactly the same as in #16772 so let's keep the general discussion in that issue.

Yes, apart from the additional question of whether scale! should exist all :).

@StefanKarpinski StefanKarpinski added this to the 1.0 milestone Oct 26, 2017
@StefanKarpinski StefanKarpinski removed the status:triage This should be discussed on a triage call label Oct 26, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 26, 2017

Triage settled on deprecating scale! in favor of mul! once the in-place/out-of-place mul! question is settled. Best!

@StefanKarpinski StefanKarpinski added the stdlib Julia's standard library label Nov 20, 2017
@JeffBezanson
Copy link
Sponsor Member

That implies that mul1! and mul2! should be exported, since scale! currently is.

@JeffBezanson
Copy link
Sponsor Member

Is anybody working on this, or should I take a crack at it?

@andreasnoack
Copy link
Member

Let me do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] domain:linear algebra Linear algebra stdlib Julia's standard library
Projects
None yet
Development

No branches or pull requests

4 participants