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

Fix #16519 by adding missing convert methods #17848

Merged
merged 1 commit into from
Aug 29, 2016
Merged

Fix #16519 by adding missing convert methods #17848

merged 1 commit into from
Aug 29, 2016

Conversation

Evizero
Copy link
Contributor

@Evizero Evizero commented Aug 5, 2016

Corresponding issue: #16519


This commit will allow a user to call:

convert(Vector, my_abstractvector_of_eltype_Foo)
convert(Matrix, my_abstractmatrix_of_eltype_Foo)

which is useful because

The same effect can (and could before this commit) also be
achieved in the following manner. Note how the two last lines
are unnecessarily verbose, because the eltype can be inferred

convert(Array, my_abstractvector_of_eltype_Foo)
convert(Array, my_abstractmatrix_of_eltype_Foo)
convert(Vector{Foo}, my_abstractvector_of_eltype_Foo)
convert(Matrix{Foo}, my_abstractmatrix_of_eltype_Foo)

This commit will allow a user to call:

```julia
convert(Vector, my_abstractvector_of_eltype_Foo)
convert(Matrix, my_abstractmatrix_of_eltype_Foo)
```

which is useful because
- for the case that the variable is a subarray of the same dimensionality
- for consistency with PR #17066

The same effect can (and could before this commit) also be
achieved in the following manner. Note how the two last lines
are unnecessarily verbose, because the eltype can be inferred

```julia
convert(Array, my_abstractvector_of_eltype_Foo)
convert(Array, my_abstractmatrix_of_eltype_Foo)
convert(Vector{Foo}, my_abstractvector_of_eltype_Foo)
convert(Matrix{Foo}, my_abstractmatrix_of_eltype_Foo)
```
@Evizero
Copy link
Contributor Author

Evizero commented Aug 5, 2016

cc: @Sacha0, @cstjean, @nalimilan

@Sacha0
Copy link
Member

Sacha0 commented Aug 5, 2016

Looks great!

@kshyatt kshyatt added linear algebra Linear algebra bugfix This change fixes an existing bug labels Aug 6, 2016
@kshyatt
Copy link
Contributor

kshyatt commented Aug 6, 2016

Nice! From your description, I should be able to do convert(Matrix, A::Diagonal) (or similar for any special type, please correct me if I'm wrong!), might be nice to have a test for that as well?

@Evizero
Copy link
Contributor Author

Evizero commented Aug 6, 2016

Yes, @kshyatt ! The two conditions for this to work now are basically typeof(Diagonal(rand(10,10))) <: AbstractMatrix needs to be true (which it is on my 0.4 machine at least) and convert(Matrix{Float64}, Diagonal(rand(2,2))) needs to work as intended (also does on 0.4).

I could add convert(Matrix, Diagonal(rand(2,2)) to the tests if you want. Or did you have something more elaborate in mind?

@kshyatt
Copy link
Contributor

kshyatt commented Aug 6, 2016

I could add convert(Matrix, Diagonal(rand(2,2)) to the tests if you want. Or did you have something more elaborate in mind?

You might check out the tests in test/linalg/special.jl (testing interconversion between special matrix types) for an example of a relatively easy way to test your (cool!) functionality for all this. Since Diagonal is a subtype of nearly all the other weird little types we have running around, it's not super verbose and I'm a sucker for more thorough testing.

@Evizero
Copy link
Contributor Author

Evizero commented Aug 6, 2016

I shall do that soon! thanks for the pointers.

@kshyatt
Copy link
Contributor

kshyatt commented Aug 6, 2016

Great! Please feel free to ask for help/explanations if you need them :)

@Sacha0
Copy link
Member

Sacha0 commented Aug 7, 2016

Conversions from matrix types for which there exist full methods (for example the special matrix types) to Matrix/Array should work as of #17066. For example, on master

julia> convert(Matrix, Diagonal(ones(4)))
4×4 Array{Float64,2}:
 1.0  0.0  0.0  0.0
 0.0  1.0  0.0  0.0
 0.0  0.0  1.0  0.0
 0.0  0.0  0.0  1.0

#17079 migrated full's tests to the replacement convert methods but was reverted due to issues with concatenation. The recent concatenation fixes should address those issues, hopefully enabling reinstatement of #17079. That is, the full-replacement convert methods should be independently tested again in the not-too-distant future. Hence writing tests targeting the use cases in #16519 and similar (other view-like objects, e.g. ReshapedArray?) might be more worthwhile. Thoughts? Best!

@Evizero
Copy link
Contributor Author

Evizero commented Aug 8, 2016

Yes, so it looks like the special types are taken care of test-wise, no? I went through the PRs you posted and it seems that most of the types similar to Diagonal have their own convert(::Type{Matrix}, D::Diagonal), which makes my implementation a fallback method for those types that don't. I don't think I should add tests for types ala Diagonal here, since they don't actually trigger the methods that this PR introduces

I guess the question what subtypes of abstract array (aside subarrays) are not covered by @Sacha0 refactor then.

@kshyatt
Copy link
Contributor

kshyatt commented Aug 8, 2016

I guess the question what subtypes of abstract array (aside subarrays) are not covered by @Sacha0 refactor then.

Yes, but IMO if it's going to be too much of a pain to figure this out the PR as it stands is perfectly fine.

@Evizero
Copy link
Contributor Author

Evizero commented Aug 8, 2016

Not sure how to proceed here. I did rebase on the latest master locally to see if there would be any apparent weird interactions with the related changes introduced by @Sacha0 and everything seems fine as far as I can tell.

@Sacha0
Copy link
Member

Sacha0 commented Aug 8, 2016

Yes, but IMO if it's going to be too much of a pain to figure this out the PR as it stands is perfectly fine.

I'm with @kshyatt: This is great as it stands! :)

@KristofferC KristofferC closed this Aug 8, 2016
@Evizero
Copy link
Contributor Author

Evizero commented Aug 8, 2016

so that's a no then @KristofferC ? anything in particular that I messed up or could have done differently? a little context/feedback would help me improve

@tkelman
Copy link
Contributor

tkelman commented Aug 8, 2016

That seems like it may have been a wrong-button mistake?

@kshyatt kshyatt reopened this Aug 8, 2016
@KristofferC
Copy link
Member

KristofferC commented Aug 8, 2016

Must have had the finger in the wrong place when scrolling on the phone. Sorry. I would never intentionally just close an ongoing PR without a good reason and a comment.

@nalimilan
Copy link
Member

Sorry we forgot this for so long. Since people supported this change for several weeks, I'll merge tomorrow if nobody objects.

@nalimilan nalimilan closed this Aug 29, 2016
@nalimilan nalimilan reopened this Aug 29, 2016
@nalimilan nalimilan merged commit bffeedb into JuliaLang:master Aug 29, 2016
@Evizero Evizero deleted the convert_vector branch August 29, 2016 17:28
tkelman pushed a commit that referenced this pull request Aug 29, 2016
…7848)

This commit will allow a user to call:
convert(Vector, my_abstractvector_of_eltype_Foo)
convert(Matrix, my_abstractmatrix_of_eltype_Foo)

which is useful because
- for the case that the variable is a subarray of the same dimensionality
- for consistency with PR #17066
(cherry picked from commit bffeedb)
@Evizero
Copy link
Contributor Author

Evizero commented Aug 30, 2016

🎉 first PR! babysteps

mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
…liaLang#17848)

This commit will allow a user to call:
convert(Vector, my_abstractvector_of_eltype_Foo)
convert(Matrix, my_abstractmatrix_of_eltype_Foo)

which is useful because
- for the case that the variable is a subarray of the same dimensionality
- for consistency with PR JuliaLang#17066
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants