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

RowVector support in v0.7 #531

Closed
schillic opened this issue Aug 12, 2018 · 4 comments
Closed

RowVector support in v0.7 #531

schillic opened this issue Aug 12, 2018 · 4 comments
Assignees
Labels
fix 🤕 Fix to a problem that is not too serious

Comments

@schillic
Copy link
Member

schillic commented Aug 12, 2018

This is the last missing step toward v0.7 support (#121).

In ExponentialMap.jl we have a function with this signature:

function get_row(spmexp::SparseMatrixExp{N}, i::Int)::RowVector{N} where {N}

We currently use transpose, which in v0.7 results in an object of type LinearAlgebra.Transpose, so we get an error because the type is different from the expected RowVector. In v0.6 the transpose function creates a RowVector.

I cannot find a way to create a RowVector without deprecation warning in v0.7.

To me, the suggested way to transpose a vector is either through transpose (lazy) or permutedims (explicit) (search for "transpose" here). However, the types are different from RowVector:

julia> x = [1., 2., 3.]
3-element Array{Float64,1}:
 1.0
 2.0
 3.0

julia> transpose(x)
1×3 LinearAlgebra.Transpose{Float64,Array{Float64,1}}:
 1.0  2.0  3.0

julia> permutedims(x)
1×3 Array{Float64,2}:
 1.0  2.0  3.0

The above is the only place where we use RowVector, so it might be safe to just change the return type of this function. Should we do that? To which type?
Note that we need a solution that also works in v0.6, but we can always write a helper function that does the right thing.

We can also create a Vector out of it:

julia> @time transpose(x)[:]
  0.000025 seconds (6 allocations: 288 bytes)
3-element Array{Float64,1}:
 1.0
 2.0
 3.0

# faster but one more allocation
julia> @time permutedims(x)[:]
  0.000012 seconds (7 allocations: 368 bytes)
3-element Array{Float64,1}:
 1.0
 2.0
 3.0
@schillic schillic added discussion 🗣️ Requires human input fix 🤕 Fix to a problem that is not too serious labels Aug 12, 2018
@mforets
Copy link
Member

mforets commented Aug 12, 2018

I would suggest to remove the output type annotation altogether. In v0.7 you will get a LinearAlgebra.Transpose{Float64,Array{Float64,1}} while in v0.6 a RowVector{Float64,Array{Float64,1}}.

@mforets
Copy link
Member

mforets commented Aug 12, 2018

PS: there is no performance impact on this change, and the function is type stable after inspecting with @code_warntype.

@schillic
Copy link
Member Author

Okay, since there seems to be no good solution, I agree. We should add a note in the comment, though, and maybe add a unit test to check that type in the respective version.

@schillic schillic removed the discussion 🗣️ Requires human input label Aug 12, 2018
@mforets
Copy link
Member

mforets commented Aug 12, 2018

👍 for adding a comment in the docstring

@schillic schillic self-assigned this Aug 14, 2018
schillic added a commit that referenced this issue Aug 15, 2018
#531 - Remove result type from SparseMatrixExp's 'get_row' function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix 🤕 Fix to a problem that is not too serious
Projects
None yet
Development

No branches or pull requests

2 participants