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 #13130 - concatenation of dense and sparse should be sparse. #15172

Merged
merged 2 commits into from
May 20, 2016

Conversation

pkofod
Copy link
Contributor

@pkofod pkofod commented Feb 21, 2016

Fixes #13130 .

I had to add hcat, vcat, and hvcat for Matrix. If I didn't, it would fall back to the Union{Matrix, SparseMatrixCSC method for [rand(10,10) rand(10, 10)]. There may be cleaner ways to do all of this.

Also, currently [sprand(10, 0.1) sprand(10, 10, 0.1)] returns a dense matrix. Surely, that should be sparse. I havn't implemented [rand(10) sprand(10, 10, 0.1)] yet either.

I guess it is still to be decided if this is the wanted behaviour, but I just wanted to make a PR to mirror my issue.

By the way, I am unsure what happened with the first two commits mentioned at the issue. They were supposed to be to my fork, but it doesn't say "to pkofod/julia". Is that because I've deleted that fork since?

@pkofod
Copy link
Contributor Author

pkofod commented Feb 21, 2016

It couldn't build, as I had added the *cat methods in the wrong file, but it builds locally, so I expect it to build on travis as well - although I might have killed some tests.

@ViralBShah ViralBShah added the sparse Sparse arrays label Feb 21, 2016
@pkofod pkofod force-pushed the master branch 3 times, most recently from 31f0b7d to 5247a72 Compare February 29, 2016 09:07
@pkofod
Copy link
Contributor Author

pkofod commented Feb 29, 2016

Test failure seems to be due to #15285.

I havn't done anything about the fact that [spvector spmatrix] doesn't exist, as it should probably be a different PR.

@tkelman
Copy link
Contributor

tkelman commented Mar 4, 2016

Could this be done without having to build up and save temporary S copies for all of [X[1] X[2]] and [X[1] X[2] X[3]] and [X[1] X[2] X[3] X[4]] etc?

@pkofod
Copy link
Contributor Author

pkofod commented Mar 13, 2016

Do you mean like we do when cat'ing sparse matrices?

@pkofod
Copy link
Contributor Author

pkofod commented Apr 25, 2016

I tried to address your concern @tkelman . However, my hcat and vcat are not type stable, so hvcat fails to setindex into the SparseMatrixCSC array, see https://gist.github.com/pkofod/0e8fa1bbadebca9127d68fa5ec5dd0b0 .

I would appreciate pointers as to what I am doing wrong here.

@tkelman
Copy link
Contributor

tkelman commented Apr 27, 2016

MethodError: Cannot convert an object of type Array{Float64,2} to an object of type SparseMatrixCSC{Tv,Ti<:Integer}

Seems like we could have a go at defining that conversion method?

@pkofod
Copy link
Contributor Author

pkofod commented Apr 27, 2016

Ah, now I get it! Obviously, this is going to fail if an entire row is dense. Thanks :)

@pkofod
Copy link
Contributor Author

pkofod commented Apr 27, 2016

So I pushed another version, which simply does

        tmp_rows[i] = sparse(hcat(X[(1 : rows[i]) + k]...))

I figured that would be fine, as I thought

sparse(S::SparseMatrixCSC) = S

but apparently

sparse(S::SparseMatrixCSC) = copy(S)

I just can't quite figure out how to define the convert method, as the Array I'm indexing into is of SparseMatrixCSC without any parametrization. I thought

convert(::SparseMatrixCSC, M::Matrix) = sparse(M)

Would work, but apparently not.

@nalimilan
Copy link
Member

I just can't quite figure out how to define the convert method, as the Array I'm indexing into is of SparseMatrixCSC without any parametrization. I thought

convert(::SparseMatrixCSC, M::Matrix) = sparse(M)

Would work, but apparently not.

You need convert(::Type{SparseMatrixCSC}, M::Matrix) = sparse(M)

@pkofod
Copy link
Contributor Author

pkofod commented Apr 27, 2016

You need convert(::Type{SparseMatrixCSC}, M::Matrix) = sparse(M)

Of course, thanks.

@pkofod
Copy link
Contributor Author

pkofod commented Apr 27, 2016

travis-ci timed out on osx

For what it is worth, sparse.jl actually ran successfully on OSX before the time out :)

@pkofod
Copy link
Contributor Author

pkofod commented Apr 28, 2016

This time around it was the x86_64 linux build that failed, but that was due to an apt-get error. If it needs to be run, then maybe someone can simply restart that build, or do I need to run them all again?

(thanks)

@pkofod
Copy link
Contributor Author

pkofod commented Apr 29, 2016

Doing the hvcat I realized it was using the Array(T, dims...) which was deprecated in v0.4, and has to go in this cycle, right? I figured while I was in /base/sparse I might as well update these. I could do it in all of base, and I happily volunteer to do that, but that is probably for a different PR. For now I just did it in /sparse/, and in a separate commit from the *cat changes.

@pkofod
Copy link
Contributor Author

pkofod commented Apr 29, 2016

What happened in AppVeyor?

@tkelman
Copy link
Contributor

tkelman commented Apr 29, 2016

whatever it was, it's not good:

Please submit a bug report with steps to reproduce this fault, and any error messages that follow (in their entirety). Thanks.
Exception: EXCEPTION_ACCESS_VIOLATION at 0x0 -- unknown function (ip: 0000000000000000)
while loading no file, in expression starting on line 0
unknown function (ip: FFFFFFFFFFFFFFFF)
Allocations: 904230 (Pool: 903129; Big: 1101); GC: 1

Please submit a bug report with steps to reproduce this fault, and any error messages that follow (in their entirety). Thanks.
Exception: EXCEPTION_ACCESS_VIOLATION at 0x0 -- unknown function (ip: 0000000000000000)
while loading no file, in expression starting on line 0
unknown function (ip: FFFFFFFFFFFFFFFF)
Allocations: 904230 (Pool: 903129; Big: 1101); GC: 1

though I'd be surprised if it were due to your changes, likely a preexisting bug somewhere

@pkofod
Copy link
Contributor Author

pkofod commented May 2, 2016

@tkelman any chance I could get you to restart the two failing runs? (mac on travis and 64bit on AV)

@nalimilan
Copy link
Member

Done.

@pkofod
Copy link
Contributor Author

pkofod commented May 2, 2016

Thanks. I see travis succeeded, but AV didn't run.

@nalimilan
Copy link
Member

Ah, now it says the PR has conflicts and thus cannot run.

@pkofod
Copy link
Contributor Author

pkofod commented May 2, 2016

Oh, so I should rebase and push?

Edit: Some tests had been added to sparse.jl. Rebased.

@pkofod
Copy link
Contributor Author

pkofod commented May 2, 2016

CI, a love-hate relationship

One was the DictChannel bug, one was a time-out.

@nalimilan
Copy link
Member

OS X failure is expected at the moment, I've restarted the other.

@pkofod
Copy link
Contributor Author

pkofod commented May 8, 2016

If there are any comments at this stage, I'd gladly take them.

@pkofod
Copy link
Contributor Author

pkofod commented May 19, 2016

Sorry to bother the subscribed people, I'm just curious. Is this waiting for a decision or something similar? I'd just prefer not to rebase again before I know what going on, as I've rebased a few times just for the sake of rebasing :)

@tkelman
Copy link
Contributor

tkelman commented May 19, 2016

I think we just keep forgetting to merge before new conflicts happen, sorry. I don't see anything objectionable here and agree with the change myself.

@pkofod
Copy link
Contributor Author

pkofod commented May 19, 2016

I think we just keep forgetting to merge before new conflicts happen, sorry.

No worries! I know these are busy times.

# Sparse/dense concatenation

function hcat(Xin::Union{Matrix, SparseMatrixCSC}...)
X = SparseMatrixCSC[issparse(x) ? x : sparse(x) for x in Xin]
Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah I guess we could technically use a generator here now, though I'm not sure if it would make a noticeable difference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not quite sure I know what you mean.

Copy link
Contributor

@tkelman tkelman May 20, 2016

Choose a reason for hiding this comment

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

Generators are new syntax that's like comprehensions but creating an iterator instead of an array. Though it looks like hcat'ing a comprehension generator without a splat doesn't behave quite like I thought it would.

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

Successfully merging this pull request may close these issues.

4 participants