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

Use Matrix for similar(SpecialMatrix, shape) instead of throwing #15198

Merged
merged 1 commit into from
Feb 24, 2016

Conversation

andreasnoack
Copy link
Member

and try to be consistent across the special matrices. Fixes #15193.

This has been extensively discussed in JuliaLang/LinearAlgebra.jl#271. @tkelman I think this was somewhat in line with your suggestion and I believe it will make the interaction with special matrices smoother (i.e. less throwing).

However, this doesn't have to be the last word on similar for special matrices. We can still consider splitting up the functionality in more functions later if necessary.

@tkelman
Copy link
Contributor

tkelman commented Feb 23, 2016

Will review more completely, but the quick tldr is this is doing option 1 from JuliaLang/LinearAlgebra.jl#271? It's not perfect but I think it's a decent short term stopgap.

@andreasnoack
Copy link
Member Author

Yes. It's option 1.

function similar{T}(A::Hermitian, ::Type{T})
B = similar(A.data, T)
for i = 1:size(A,1)
B[i,i] = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

real(B[i,i]) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The value shouldn't really matter here.

@tkelman
Copy link
Contributor

tkelman commented Feb 23, 2016

Can you add tests that check that the structural zeros or symmetries are respected in the "similar with specified output size" case? That's now going to some fallbacks that I can't quite trace in my head without building and introspecting on this branch.

@andreasnoack
Copy link
Member Author

Can you add tests that check that the structural zeros or symmetries are respected in the "similar with specified output size" case?

I don't understand this. When you specify the output size, you get an array of the "parent type". Most often that is a Matrix and therefore it won't have any structural zeros.

@tkelman
Copy link
Contributor

tkelman commented Feb 23, 2016

The question is whether the result of similar should be preset with the structural data from the input matrix in the case where you're losing information by returning Matrix instead of a more specialized type, or if it should always be the user's responsibility to manually iterate through even uninteresting elements to set them.

@andreasnoack
Copy link
Member Author

The structure doesn't generally have a meaning when asking for a specific shape. E.g. triangularity and symmetry can't be imposed on the rectangular matrices. In practice, I think the cases where you ask for a specific shape will be cases where you'll visit all elements.

Instead of falling back to the similar method in abstractarray.jl, we could add similar methods to each of the special matrices that initialize the elements with the structure whenever m=n, but it seems a bit arbitrary and adds quite a few extra methods so I'd like to see if there is really a need for this first.

@tkelman
Copy link
Contributor

tkelman commented Feb 23, 2016

Relying on "visit all elements" use case assumptions doesn't sit well, but this does depend on what similar is really supposed to be used for and increasingly points IMO that it should be split into better specified uses going forward. I guess we should try this out for now though.

You can do triangular (trapezoidal) rectangular matrices, I think symmetry is the main structural property that imposes actual size constraints mathematically as opposed to just as current implementation limitations.

@mbauman
Copy link
Member

mbauman commented Feb 23, 2016

This seems like the right direction given our definition and use of similar.

The next question, then, is if that definition is actually useful for generic programming… and in what cases. Is it worth keeping similar for the new-shape case? We've been using similar in this manner for the non-scalar fallbacks, and almost all array types have needed to change to returning an Array in order to use those fallbacks. Those that haven't end up implementing their own specializations of the algorithm anyways (e.g., sparse).

As far as I'm aware, the only array types that return a non-Array when requesting a new shape from similar and still do something reasonable in the non-scalar indexing fallbacks are StructsOfArrays/NullableArrays. I suppose this makes sense… because instead of adding or exploiting structural storage information, those array types do something special to the element type itself. So they can still do something useful in the different-shape case.


# getindex
## Linear indexing
for i = 1:length(A1)
@test A1[i] == full(A1)[i]
end
@test isa(A1[2:4,1], Matrix)
Copy link
Contributor

Choose a reason for hiding this comment

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

Vector ?

@kshyatt kshyatt added the linear algebra Linear algebra label Feb 23, 2016
try to be consistent across the special matrices. Fixes #15193.
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.

Triangular matrix indexing bug
4 participants