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

there is no full #24147

Merged
merged 1 commit into from
Oct 19, 2017
Merged

there is no full #24147

merged 1 commit into from
Oct 19, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Oct 14, 2017

After #24137, #24138, and the long series of prior full disambiguations/rewrites, only thirteen full calls remain. All such calls are on [Unit](Upper|Lower)Triangular/Symmetric/Hermitian matrices and unambiguously mean copy!(similar(parent(A)), A). A number of options for eliminating these calls follow below.

This pull request implements the simplest, most conservative option: replace the remaining calls with copy!(similar(parent(A)), A).

Another option is to introduce the generic function discussed in #23270. #24148 provides a sketch of this option. (After some thought, that generic function has (in an abstract sense) the semantics of widen. So presently #24148 extends widen instead of introducing a new exported name.) But this approach seems heavy handed now: Thirteen calls to copy!(similar(parent(A)), A) in a specific context don't alone seem worth exporting a new generic name from Base (nor worth extending an existing Base export in a major way). The generic function in #23270/#24148 may find other uses, but playing with those other uses / carefully vetting behavior prior to introducing such functionality seems wise. And we can introduce such functionality after 1.0, but we can't change or remove it in the 1.x series if we inject it now.

In any case, copy!(similar(parent(A)), A) isn't bad to write on the few occasions one needs to, is quite clear, and provides more opportunity for optimization than existing alternatives. So at this point I err on the (conservative) side of the approach in this pull request. If we really need a new name, we can introduce it later. Best!

(Also note: In #24137 (comment) Andreas and I discuss fleshing out the (unexported) full!(A::Union{[Unit](Upper|Lower)Triangular,Symmetric,Hermitian}) (which fills A's parent with A's contents and returns the parent) and renaming it to better reflect is semantics (e.g. to fillparent!(A)). Then one could write either copy!(similar(parent(A)), A) or fillparent!(copy(A)) for this operation. And if it proves necessary, introducing an unexported filledparent(A) or so would then be natural.)

@Sacha0 Sacha0 added the domain:linear algebra Linear algebra label Oct 14, 2017
@Sacha0 Sacha0 added this to the 1.0 milestone Oct 14, 2017
@@ -426,7 +426,7 @@ scale!(c::Number, A::Union{UpperTriangular,LowerTriangular}) = scale!(A,c)
+(A::UnitLowerTriangular, B::LowerTriangular) = LowerTriangular(tril(A.data, -1) + B.data + I)
+(A::UnitUpperTriangular, B::UnitUpperTriangular) = UpperTriangular(triu(A.data, 1) + triu(B.data, 1) + 2I)
+(A::UnitLowerTriangular, B::UnitLowerTriangular) = LowerTriangular(tril(A.data, -1) + tril(B.data, -1) + 2I)
+(A::AbstractTriangular, B::AbstractTriangular) = full(A) + full(B)
+(A::AbstractTriangular, B::AbstractTriangular) = copy!(similar(parent(A)), A) + copy!(similar(parent(B)), B)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definition could be simplified by widening only one of the arguments and then redispatching. That approach would likely be an improvement in any case.

@@ -436,15 +436,15 @@ scale!(c::Number, A::Union{UpperTriangular,LowerTriangular}) = scale!(A,c)
-(A::UnitLowerTriangular, B::LowerTriangular) = LowerTriangular(tril(A.data, -1) - B.data + I)
-(A::UnitUpperTriangular, B::UnitUpperTriangular) = UpperTriangular(triu(A.data, 1) - triu(B.data, 1))
-(A::UnitLowerTriangular, B::UnitLowerTriangular) = LowerTriangular(tril(A.data, -1) - tril(B.data, -1))
-(A::AbstractTriangular, B::AbstractTriangular) = full(A) - full(B)
-(A::AbstractTriangular, B::AbstractTrianglar) = copy!(similar(parent(A)), A) - copy!(similar(parent(B)), B)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise here, this definition could be simplified by widening only one of the arguments and then redispatching. That approach would likely be an improvement in any case.

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 17, 2017

Thanks @fredrikekre! Absent objections or requests for time, I plan to merge these changes this evening or tomorrow morning. Best!

@ViralBShah
Copy link
Member

ViralBShah commented Oct 17, 2017

200w_d

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 17, 2017

Pleased that someone caught the reference! :D

@Sacha0 Sacha0 merged commit 01fc2dd into JuliaLang:master Oct 19, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 19, 2017

Thanks all!

@Sacha0 Sacha0 deleted the thereisnofull branch October 19, 2017 01:28
@andreasnoack
Copy link
Member

Sorry for the delay. Have had a bit of a backlog. I don't like this. A poorly named slightly ambiguously defined generic function is still better than repeated use of copy!(similar(parent(A)), A). I also believe that general view during triage back in August was that a generic function had a role to play here. Generic functions like full (and similar) might be hard to define and have poor/misleading names but they are useful. The quest for generic programming purity where any generic function has an unambiguous meaning whatever the input types is not making the language easier to use. I'll conjecture that there are tons of these generic programming ambiguities left all over Base if we start looking carefully. At least they are there in LinAlg. E.g. sparse Cholesky and QR are doing pivoting by default whereas the dense are not. Is it then even the same operation from a purist generic programming perspective or should we have cholfactLAPACK(StridedMatrix) and cholfactSuiteSparse(SparseMatrixCSC) to avoid ambiguities? I think we should continue to have a more pragmatic approach. At least as long as no good alternatives are found.

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 19, 2017

No worries re. the delay! :)

Some abstraction over copy!(similar(parent(A)), A) for these former uses of full (as discussed in #23270 / on the triage call and explored in #24148) would be great. Unfortunately, consensus regarding that abstraction's semantics and name has not congealed yet. And as @mbauman points out in #24148 and I mention above, deferring introduction of that abstraction till we sort out a number of coupled issues would be advantageous. Fortunately, however, we can introduce that abstraction at any time and clean this up then! :) (In contrast, however, we cannot deprecate full later. And with these last few uses gone, we can finally do so.)

Till we have that abstraction in all its glory and splendour, I suggest we carry through #24137 (comment) (the fillparent! plan) and perhaps provide an also-unexported copying variant (see OP). fillparent! / its copying variant is well defined and precisely the abstraction necessary in these cases.

Best! :)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants