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

parametrize Tridiagonal on the wrapped vector type #23154

Merged
merged 1 commit into from
Aug 21, 2017
Merged

Conversation

fredrikekre
Copy link
Member

Last PR in this series. This is definitely not as straight forward as #22718, #22925 and #23035 so please review carefully. It took some thinking to do this in a nice way, hence I saved it for last. I am quite pleased with the current implementation.

The internal field .du2 was only used in lufact!, and for all other operations it should have been considered hidden. Since we never initialize that field otherwise, other operations on Tridiagonal should allocate less after this change:
@nanosoldier runbenchmarks("linalg", vs = ":master")

The other good thing with not initializing that field always, is that we don't need to use similar, to construct it, since that is not applicable for all AbstractVectors (e.g. Tridiagonal(1:2, 1:3, 1:2) would not work, how to construct the .du2 field?). Now it is only needed in lufact!.

@fredrikekre fredrikekre added breaking This change will break code deprecation This change introduces or involves a deprecation linear algebra Linear algebra labels Aug 6, 2017
@fredrikekre
Copy link
Member Author

Meh, had to fix something so lets redo this
@nanosoldier runbenchmarks("linalg", vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @ararslan

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @ararslan

NEWS.md Outdated
wrapped vectors, allowing `Diagonal`, `Bidiagonal` and `SymTridiagonal` matrices with
arbitrary `AbstractVector`s ([#22718], [#22925], [#23035]).
* `Diagonal`, `Bidiagonal`, `Tridiagonal` and `SymTridiagonal` are now parameterized on the
type of the wrapped vectors, allowing `Diagonal`, `Bidiagonal` and `SymTridiagonal`
Copy link
Member

Choose a reason for hiding this comment

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

Missing a Tridiagonal?

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 catch!

A.uplo == 'U' ? Tridiagonal(z, convert(Vector{T},A.dv), convert(Vector{T},A.ev)) : Tridiagonal(convert(Vector{T},A.ev), convert(Vector{T},A.dv), z)
dv = convert(AbstractVector{T}, A.dv)
ev = convert(AbstractVector{T}, A.ev)
z = fill!(similar(ev), zero(T))
Copy link
Member

Choose a reason for hiding this comment

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

To check my understanding, converting a Bidiagonal over Ranges to a Tridiagonal should fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right..., Yea I guess that’s a consequence of this series of PR's? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're converting from Diagonal or Bidiagonal or SymTridiagonal that have fewer diagonal vectors and they already have to be a consistent type within those structs, can't we use that same parameter to know which new diagonal vector type to fill in when converting to Tridiagonal?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the case of a Range: You can not create a Range of a given length with only zeros, right? That is the only failure case here I think...
Using Ranges results in other problems as well, so I guess you will just have to convert to a vector format that supports a larger set of functions.

@fredrikekre
Copy link
Member Author

Any more comments?

@fredrikekre
Copy link
Member Author

Rebased to run through CI once more before merging since its been over a week.

@fredrikekre
Copy link
Member Author

InterruptException on macos, restarted. Also, there are 3200 lines of depwarns in the logs from Documenter and friends, any chance we could upgrade those dependencies? :)

@tkelman
Copy link
Contributor

tkelman commented Aug 21, 2017

sure, if there are new versions to upgrade to

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code deprecation This change introduces or involves a deprecation linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants