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 vec and diag for RowVector #25237

Closed
wants to merge 1 commit into from
Closed

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Dec 21, 2017

I propose to fix two issues with RowVector.

  1. vec on RowVector can be made more efficient by defining a specialized method
  2. currently diag on RowVector returns RowVector and it should return a Vector; in this PR this is fixed

The reason for fixing diag is the following situation:

julia> [2 2; 2 2] / [2]
ERROR: StackOverflowError:

whereas it should be [1.0 1.0; 1.0 1.0] as it is the result of reshape([2], 1, 1) \ [2 2; 2 2].

Additionally (I have not touched this because it is probably intentional - if not I propose to fix it):

@propagate_inbounds getindex(rowvec::RowVector, i) = transpose(rowvec.vec[i])

which means that indexing into RowVector by a vector produces RowVector. This is inconsistent with the general behavior that indexing into e.g. Matrix by a vector produces Vector, e.g.

reshape([1,2], 1, 2)[[1]]

is a Vector, but

[1,2]'[[1]]

is a RowVector.

@fredrikekre
Copy link
Member

All of this seem to work on master?

julia> [2 2; 2 2] / [2]
2×2 Array{Float64,2}:
 1.0  1.0
 1.0  1.0

julia> reshape([1,2], 1, 2)[[1]]
1-element Array{Int64,1}:
 1

julia> [1,2]'[[1]]
1-element Array{Int64,1}:
 1

Also, RowVector will be gone after #25217

@bkamins
Copy link
Member Author

bkamins commented Dec 21, 2017

@fredrikekre sorry for the mess - I keep forgetting that recent pace of improvements in Julia is so fast and checked the code on older version. Closing.

@bkamins bkamins closed this Dec 21, 2017
@Sacha0
Copy link
Member

Sacha0 commented Dec 21, 2017

No worries @bkamins! Rather, much thanks for your wonderful recent efforts! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants