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.

Sign up for GitHub

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

Jump to bottom

Fallback for copying views into sparse matrices #30895

Merged
merged 4 commits into from
Jan 31, 2019
Merged

Fallback for copying views into sparse matrices #30895

merged 4 commits into from
Jan 31, 2019

Conversation

mkborregaard
Copy link
Contributor

This is a more specialized version of #30889, requested for backporting.

@mkborregaard mkborregaard mentioned this pull request Jan 29, 2019
Merged
@mkborregaard
Copy link
Contributor Author

Once again a bit in doubt about where to best put this code.

@ViralBShah ViralBShah added the sparse Sparse arrays label Jan 29, 2019
@ViralBShah
Copy link
Member

All tests failing seem to suggest something is wrong (I haven't looked inside yet).

@mkborregaard
Copy link
Contributor Author

Yeah, UndefVarError(:SparseArray). I might have put this the wrong place.

@mkborregaard
Copy link
Contributor Author

I just copied that code from the other PR, sorry - I could replace with

Base.copy(a::SubArray{<:Any,<:Any,<:Union{SparseVector, SparseMatrixCSC}}) = a.parent[a.indices...]

?

@mbauman
Copy link
Member

mbauman commented Jan 30, 2019 •
edited
Loading

Yeah, sorry, I should have made it clear that was psuedocode! Using a Union like that is probably best. As far as where it should go... maybe here would make sense? It's where we are defining indexing for combinations of sparse vector/matrix, which seems relevant.

Edit: oh, but if you want to use the SparseVecOrMat typealias you'll also have to move that to come earlier — it'd make sense to define it near the top of the sparsevector file, I suppose.

@mkborregaard
Copy link
Contributor Author

OK, I've moved it there and replaced SparseVecOrMat with the explicit Union

@mkborregaard
Copy link
Contributor Author

Failure on the Travis Mac build looks unrelated/spurious, it's a GitError "can't connnect to github"

@mbauman mbauman added the performance Must go faster label Jan 31, 2019
@StefanKarpinski StefanKarpinski added the triage This should be discussed on a triage call label Jan 31, 2019
@JeffBezanson JeffBezanson added backport pending 1.1 and removed triage This should be discussed on a triage call triage backport pending 1.0 labels Jan 31, 2019
@mbauman mbauman merged commit d6bcb5a into JuliaLang:master Jan 31, 2019
KristofferC pushed a commit that referenced this pull request Feb 11, 2019
@mkborregaard @KristofferC
Fallback for copying views into sparse matrices (#30895)
2fcf987 
* Fallback for copying views into sparse matrices

* SparseArray --> SparseVecOrMat

* move copy method to sparsevector

* use the union explicitly

(cherry picked from commit d6bcb5a)
@KristofferC KristofferC mentioned this pull request Feb 11, 2019
Merged
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbauman mbauman mbauman approved these changes

Assignees
No one assigned
Labels
performance Must go faster sparse Sparse arrays
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mkborregaard @ViralBShah @mbauman @StefanKarpinski @JeffBezanson