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 Diagonal on the wrapped vector type #22718

Merged
merged 1 commit into from
Jul 17, 2017

Conversation

fredrikekre
Copy link
Member

Because why not? Ref e.g. JuliaArrays/StaticArrays.jl#235

IIUC this could potentially be bad for people that now have ::Diagonal{T} fields in user defined types though, since theses types will not be fully parametrized?

If this is accepted, the same thing should probably be done with Bidiagonal and [Sym]Tridiagonal.

@yuyichao yuyichao added the breaking This change will break code label Jul 9, 2017
@fredrikekre
Copy link
Member Author

fredrikekre commented Jul 9, 2017

Another idea, define something like

abstract type AbstractDiagonal{T} <: AbstractMatrix{T} end
struct Diagonal{T} <: AbstractDiagonal{T}
    diag::Vector{T}
end

and relax methods in base to AbstractDiagonal. Then users can subtype AbstractDiagonal like we currently subtype AbstractArray for our own types. The downside is that all users then need to use the same fieldname for the diagonal, since there are lots of direct field access to the vector. Or we need a standard way of obtaining the diagonal.

@fredrikekre
Copy link
Member Author

Actually, we have diag for that, so all D.diag calls can be diag(D) instead.

1 ⋅ ⋅
⋅ 5 ⋅
⋅ ⋅ 9
```
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

no blank between docstring and what it's documenting

Copy link
Member Author

Choose a reason for hiding this comment

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

Woops, fixed.

@fredrikekre
Copy link
Member Author

fredrikekre commented Jul 10, 2017

The thing with my alternative approach is that it is not enough to subtype AbstractVector but you have to introduce your own diagonal type and subtype that manually. The beauty with julia is usually that things JustWork™ with your own types, so I think this change is worth it.

Also, I searched through all registered packages, and this diff would fix the breakage part of this PR:

diff --git a/src/LowRankMatrix.jl b/src/LowRankMatrix.jl
index 7989736..a59da0e 100644
--- a/src/LowRankMatrix.jl
+++ b/src/LowRankMatrix.jl
@@ -8,7 +8,7 @@ Store the singular value decomposition of a matrix:
 """
 immutable LowRankMatrix{T} <: AbstractLowRankMatrix{T}
     U::Matrix{T}
-    Σ::Diagonal{T}
+    Σ::Diagonal{T,Vector{T}}
     V::Matrix{T}
     temp::Vector{T}
 end

Edit: And for Bidiagonal and [Sym]Tridiagonal, there is no usage of that in [mutable] structs in any registered package.

@tkelman
Copy link
Contributor

tkelman commented Jul 11, 2017

not sure whether these are covered in benchmarks but let's see @nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

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

@@ -312,7 +312,7 @@ end
end

# allow construct from range
@test Diagonal(linspace(1,3,3)) == Diagonal([1.,2.,3.])
@test Diagonal(linspace(1,3,3)) Diagonal([1.,2.,3.])
Copy link
Member

Choose a reason for hiding this comment

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

These should be elementwise equal - is this better expressed as all(Diagonal(linspace(1,3,3)) .== Diagonal([1.0,2.0,3.0]))?

I must admit to being surprised that 1:2 == [1,2] is false in the first place, as other AbstractVectors are compared elementwise. I hunted this down to #5778 which seems to be a bit of a thorny issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will presumably be fixed by #16401? @nalimilan

Copy link
Member

Choose a reason for hiding this comment

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

Right. As you say, that's a thorny issue... ;-) Until then, better use all indeed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

superficially lgtm! :)

@fredrikekre
Copy link
Member Author

Rebased.

@fredrikekre
Copy link
Member Author

Rebased.

@KristofferC
Copy link
Member

Is this first use of triangular dispatch in base? Probably not...

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants