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

yak shaving, full deprecation style #23838

Merged
merged 3 commits into from
Sep 26, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Sep 23, 2017

tl;dr
This pull request eliminates a few uses of full from base/linalg/special.jl, via a minor yak shave. Ref. JuliaLang/LinearAlgebra.jl#229, JuliaLang/LinearAlgebra.jl#231, #18850, and linked threads.

details
The eliminated uses of full were part of ad hoc checks for upper bidiagonal, lower bidiagonal, and tridiagonal band structure in conversions between structured and/or annotated matrix types.

The first commit generalizes existing and introduces new (not exported) generic functions that perform the necessary upper bidiagonal, lower bidiagonal, and tridiagonal band structure checks. Specifically, the first commit extends istri[u|l](A) to istri[u|l](A, k) matching tri[u|l](A, k), implements isbanded(A, kl, ku) as a short child of istri[u|l](A, k), (re)implements isdiag(A), isbidiag[u|l](A), and istridiag(A) as short children of isbanded(A, ul, uk), and tests and documents all those functions.

The second commit eliminates the uses of full in question via the functions provided by the first commit.

The third commit then cleans up the complete set of surrounding structured and/or annotated matrix interconversion methods, at a pleasing LOC reduction.

Best!

@Sacha0 Sacha0 added the linear algebra Linear algebra label Sep 23, 2017
@Sacha0 Sacha0 added this to the 1.0 milestone Sep 23, 2017
return false
for j in 1:min(n, m + k - 1)
for i in max(1, j - k + 1):m
iszero(A[i, j]) || return false
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe @inbounds?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not certain this is safe for arbitrary AbstractArrays, for example OffsetArrays, and so hesitate to add an @inbounds?

Copy link
Member

Choose a reason for hiding this comment

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

Eventually, these functions should use either be restricted to StridedMatrix or the index calculations be changed to work with OffsetArrays.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call :). Do you have thoughts on the best way to generalize these loops? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

The easiest might be to reuse the existing loops and then use the i and j to index into the actual indices for the array, e.g. ii = indices(A, 1)[i], and then index with ii and jj.

@fredrikekre fredrikekre self-requested a review September 23, 2017 12:27
@fredrikekre
Copy link
Member

Looks great, I like the isbanded function. I am a bit skeptical to isbidiag[u|l] and istridiag though. I am not sure they are very useful (seem to be used only a couple of times in this PR, which should indicate something?), it is simple enough to do isbanded(A, 0, 1) / isbanded(A, -1, 0) / isbanded(A, -1, 1)? With introduction of isbanded we could perhaps instead @deprecate isdiag(A) isbanded(A, 0, 0) rather than introducing more specific functions like that? Thoughts? 😄

@StefanKarpinski
Copy link
Member

I think isdiag is traditional and common enough that we should keep it. 100% agree that isbidiag, istridiag etc. should just be expressed using isbanded.

@Sacha0
Copy link
Member Author

Sacha0 commented Sep 25, 2017

Cheers, agreed :).

@Sacha0
Copy link
Member Author

Sacha0 commented Sep 26, 2017

Nixed isbidiag[u|l] and istridiag. Thoughts? Thanks!

@andreasnoack andreasnoack merged commit a05ced7 into JuliaLang:master Sep 26, 2017
@fredrikekre fredrikekre removed their request for review September 26, 2017 07:49
@Sacha0 Sacha0 deleted the isbandfullconv branch September 26, 2017 19:58
@Sacha0
Copy link
Member Author

Sacha0 commented Sep 26, 2017

Thanks all! :)

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

Successfully merging this pull request may close these issues.

5 participants