Skip to content

Add hstack, vstack, and bmat to cupyx.scipy.sparse#2665

Merged
niboshi merged 29 commits intocupy:masterfrom
cjnolet:fea-bmat_vstack_hstack
Mar 6, 2020
Merged

Add hstack, vstack, and bmat to cupyx.scipy.sparse#2665
niboshi merged 29 commits intocupy:masterfrom
cjnolet:fea-bmat_vstack_hstack

Conversation

@cjnolet
Copy link
Member

@cjnolet cjnolet commented Nov 20, 2019

I started with the corresponding functions from scipy.sparse and made some modifications to make them work with cupy. Unfortunately, since cupy.sparse.coo_matrix does not take a dense matrix as an argument to its constructor, the example in the pydocs will need to change slightly.

I will be working to add tests next.

@grlee77
Copy link
Member

grlee77 commented Nov 20, 2019

Nice start, @cjnolet. I left a few initial comments, but haven't done a close review

@cjnolet cjnolet changed the title [WIP] Add hstack, vstack, and bmat to cupy.sparse [REVIEW] Add hstack, vstack, and bmat to cupy.sparse Dec 9, 2019
@cjnolet
Copy link
Member Author

cjnolet commented Dec 9, 2019

@niboshi, I believe this PR is ready for review.

@cjnolet
Copy link
Member Author

cjnolet commented Dec 19, 2019

@niboshi, Thank you for the review.

I've implemented your review feedback. I believe this is ready for re-review.

@dloney
Copy link
Contributor

dloney commented Jan 10, 2020

What's the status with this? I could use this functionality as well, and I'm happy to help get the feature finalized.

@cjnolet
Copy link
Member Author

cjnolet commented Jan 14, 2020

@dloney, I believe this PR is ready to go. Awaiting further feedback, or merge, from @niboshi.

@niboshi niboshi added this to the v8.0.0a1 milestone Jan 22, 2020
@cjnolet
Copy link
Member Author

cjnolet commented Jan 25, 2020

@niboshi, checking in. We really need this feature in RAPIDS. Do you have any further change requests on this PR?

@cjnolet
Copy link
Member Author

cjnolet commented Feb 5, 2020

@grlee77 @niboshi

Ping. It looks like there are other folks interested in this PR as well. We require this feature in RAPIDS and would appreciate a re-review.

Copy link
Member

@niboshi niboshi left a comment

Choose a reason for hiding this comment

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

I'm sorry for delay. Please check.

Co-Authored-By: niboshi <niboshi000@gmail.com>
@emcastillo emcastillo modified the milestones: v8.0.0a1, v8.0.0b1 Feb 14, 2020
@cjnolet
Copy link
Member Author

cjnolet commented Mar 2, 2020

@niboshi requested changes have been made

@cjnolet
Copy link
Member Author

cjnolet commented Mar 3, 2020

@niboshi requested changes have been made

@niboshi
Copy link
Member

niboshi commented Mar 3, 2020

Thanks!
Jenkins, test this please

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 997bc57:

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 997bc57, target branch master) failed with status FAILURE.

@cjnolet
Copy link
Member Author

cjnolet commented Mar 5, 2020

@niboshi the requested changes have been made. I used csr_matrix instead of coo_matrix and used array() to put the Python lists on device.

@niboshi
Copy link
Member

niboshi commented Mar 6, 2020

Thanks! Why would you need to add float32?

@cjnolet
Copy link
Member Author

cjnolet commented Mar 6, 2020

I can use decimals if you prefer. I guess I'm just in the habit of using single precision float wherever possible because of the performance implications of double precision. Let me know if you'd like me to change it and I'll do so quickly.

@cjnolet
Copy link
Member Author

cjnolet commented Mar 6, 2020

@niboshi, I updated it anyways. Makes the docs look cleaner.

@niboshi
Copy link
Member

niboshi commented Mar 6, 2020

We don't use float32 explicitly in other example codes.
Thank you for the fix!
Jenkins, test this please.

@pfn-ci-bot

This comment has been minimized.

@cjnolet
Copy link
Member Author

cjnolet commented Mar 6, 2020

Forgive me, @niboshi, I should have regenerated the output for the example after modifying it.

@niboshi
Copy link
Member

niboshi commented Mar 6, 2020

NP, I didn't notice either. 😅
Jenkins, test this please

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 171ff88:

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 171ff88, target branch master) succeeded!

@niboshi niboshi changed the title [REVIEW] Add hstack, vstack, and bmat to cupy.sparse Add hstack, vstack, and bmat to cupyx.scipy.sparse Mar 6, 2020
@niboshi
Copy link
Member

niboshi commented Mar 6, 2020

Thank you so much!

@niboshi niboshi merged commit 463a353 into cupy:master Mar 6, 2020
@cjnolet
Copy link
Member Author

cjnolet commented Mar 6, 2020

Thank you, @niboshi!

@kmaehashi kmaehashi added the cat:feature New features/APIs label Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cat:feature New features/APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants