Skip to content
This repository has been archived by the owner on Mar 12, 2021. It is now read-only.

Implement a few sparse array basics #572

Merged
merged 4 commits into from
Apr 24, 2020
Merged

Implement a few sparse array basics #572

merged 4 commits into from
Apr 24, 2020

Conversation

janEbert
Copy link
Contributor

@janEbert janEbert commented Jan 25, 2020 •
edited
Loading

Namely getindex, nnz, nonzeros and nonzeroinds. These also fix the show methods for SparseCuVector and SparseCuMatrixCS{C,R} (albeit using scalar indexing).

#451

@maleadt
Copy link
Member

maleadt commented Jan 28, 2020

bors r+

bors bot added a commit that referenced this pull request Jan 28, 2020
@bors @janEbert
Merge #572
e899e29 
572: Implement a few sparse array basics r=maleadt a=janEbert

Namely `nnz`, `nonzeros` and `nonzeroinds`. The last two also fix the `show` method for `SparseCuVector` (albeit using scalar indexing).

#451

Co-authored-by: janEbert <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jan 28, 2020

Build failed

@maleadt
Copy link
Member

maleadt commented Jan 28, 2020

You need to wrap in calls to Array yourself to download the values before comparison, and avoid scalar iteration.

@janEbert
Copy link
Contributor Author

So for the tests I should write, for example,
@test Array(nonzeros(d_x)) == nonzeros(x)?
Or do you mean I should re-implement show to wrap the returned values in Array?
Or something else entirely?

@maleadt
Copy link
Member

maleadt commented Jan 28, 2020

No @test Array(nonzeros(d_x)) == nonzeros(x) is fine. There's were some other CI failures too, though.

@amontoison
Copy link
Member

@janEbert bump
Very useful pull request! show doesn't work for an CuSparseMatrixCSC otherwise.

@janEbert
Copy link
Contributor Author

janEbert commented Apr 2, 2020

Hey! Thanks for the interest and reminder, I kind of forgot about this.
I'll try to fix everything today. :)

@janEbert
Copy link
Contributor Author

janEbert commented Apr 2, 2020

Hey, this is fixed (fingers crossed ;) ) and can be retried.
Showing CuSparseMatrixCS{C,R} still won't work though as I need to either implement getindex or write a GPU-compliant show for CuSparseMatrixCS{C,R}.

I had already started work on implementing getindex and will commit it to this PR when done (unless the PR has already been merged then).

@janEbert
Implement getindex
9fb78a8 
This also fixes `show` for sparse GPU matrices.
@maleadt
Copy link
Member

maleadt commented Apr 2, 2020

bors try

@janEbert
Copy link
Contributor Author

janEbert commented Apr 2, 2020 •
edited
Loading

getindex is added now. However, the commits don't show up.

bors bot added a commit that referenced this pull request Apr 2, 2020
@bors
Try #572:
3e3c404
@bors
Copy link
Contributor

bors bot commented Apr 2, 2020

try

Build succeeded

@maleadt
Copy link
Member

maleadt commented Apr 3, 2020

I had already started work on implementing getindex and will commit it to this PR when done (unless the PR has already been merged then).

Great! Let's keep this PR open then.

@janEbert
Copy link
Contributor Author

janEbert commented Apr 4, 2020 •
edited
Loading

It's really weird; going to the commits of my PR branch, I see the commits were uploaded correctly but they still aren't listed here; what should I do?

@janEbert
Copy link
Contributor Author

janEbert commented Apr 5, 2020

We can close this so I can open a new PR with the same branch that (hopefully) does not have the issue. Or this is merged and I open a new PR for getindex. Or we try and fix it another way. Whatever you want. :)

@janEbert janEbert changed the base branch from master to stable April 5, 2020 11:37
@janEbert janEbert changed the base branch from stable to master April 5, 2020 11:37
@janEbert
Copy link
Contributor Author

janEbert commented Apr 5, 2020

Sorry for the triple post; changing base branches did the trick.

@amontoison
Copy link
Member

@janEbert, Is it ready to be merged?

@janEbert
Copy link
Contributor Author

Absolutely!

@maleadt
Copy link
Member

maleadt commented Apr 23, 2020

Oh, I thought you wanted to push additional changes to chis branch. Let's merge it then:
bors r+

bors bot added a commit that referenced this pull request Apr 23, 2020
@bors @janEbert
Merge #572
7b24100 
572: Implement a few sparse array basics r=maleadt a=janEbert

Namely `getindex`, `nnz`, `nonzeros` and `nonzeroinds`. These also fix the `show` methods for `SparseCuVector` and `SparseCuMatrixCS{C,R}` (albeit using scalar indexing). 

#451

Co-authored-by: janEbert <[email protected]>
@bors
Copy link
Contributor

bors bot commented Apr 23, 2020

Build failed:

@janEbert
Copy link
Contributor Author

Oh, I thought you wanted to push additional changes to chis branch.

Absolutely, though I was quickly done with those. Afterwards, there was a Github bug preventing the changes from showing up.

The errors were due to me using Julia 1.4 begin indexing. I de-sugared those operations to use firstindex now so please try again.

@maleadt
Copy link
Member

maleadt commented Apr 24, 2020

bors try

bors bot added a commit that referenced this pull request Apr 24, 2020
@bors
Try #572:
3615297
@bors
Copy link
Contributor

bors bot commented Apr 24, 2020

try

Build failed:

@maleadt
Copy link
Member

maleadt commented Apr 24, 2020

CI failure unrelated.

@maleadt maleadt merged commit c8d9a9b into JuliaGPU:master Apr 24, 2020
@janEbert janEbert deleted the sparse branch May 13, 2020 20:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@janEbert @maleadt @amontoison