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

Fix sparse type parameter (fixes #22110) #22111

Merged
merged 2 commits into from
May 29, 2017
Merged

Fix sparse type parameter (fixes #22110) #22111

merged 2 commits into from
May 29, 2017

Conversation

jebej
Copy link
Contributor

@jebej jebej commented May 28, 2017

No description provided.

jebej
jebej commented May 28, 2017
View reviewed changes
base/sparse/sparsematrix.jl
@@ -1418,7 +1423,7 @@ if not specified.
`sparse(α*I, m, n)` can be used to efficiently create a sparse
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this supposed to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, on 0.6

KristofferC
KristofferC reviewed May 28, 2017
View reviewed changes
Copy link
Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

Was it needed to put in so many places just to fix spzeros and speye? Or are the other ones fixing other things?

@jebej
Copy link
Contributor Author

jebej commented May 28, 2017

It fixes similar as well, which I have checked, and I assume (haven't tested it) it would fix _mapreducezeros, since I just made the same change.

@jebej
Copy link
Contributor Author

jebej commented May 28, 2017

It seems to have passed the sparse tests and all those functions are being tested, so I think it's okay.

musm
musm reviewed May 28, 2017
View reviewed changes
base/sparse/sparsematrix.jl Outdated
@@ -292,7 +292,10 @@ function copy!(A::SparseMatrixCSC, B::SparseMatrixCSC)
return A
end

similar(S::SparseMatrixCSC, Tv::Type = eltype(S)) = SparseMatrixCSC(S.m, S.n, copy(S.colptr), copy(S.rowval), Vector{Tv}(length(S.nzval)))
function similar(S::SparseMatrixCSC, ::Type{Tv} = eltype(S)) where {Tv}
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the braces around {Tv}

Copy link
Contributor

Choose a reason for hiding this comment

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

same for speye_scaled and _mapreducezeros

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I don't know how to fix this without opening an other pull request :( I just edited the file in Github... I hope that's not a huge issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the same git branch make these changes then commit then and push them. They will show up here

Copy link
Member

Choose a reason for hiding this comment

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

Editing in github is not going to be convenient for contributing to Julia. Git can be a bit difficult at first, but you can pretty easily get into the groove. Basically, once you have a branch set up to be a PR, you can keep committing to the same branch. When we merge your PR, we will combine into one big commit.

Opening a new PR every time creates a lot of noise otherwise, and splits the discussion and reviews into too many places.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe better keep the short function syntax for one-liners like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a pretty long line

Copy link
Member

Choose a reason for hiding this comment

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

Of course you would split it into two lines (as in other places in the file).

@kshyatt kshyatt added sparse Sparse arrays types and dispatch Types, subtyping and method dispatch labels May 29, 2017
@kshyatt
Copy link
Contributor

kshyatt commented May 29, 2017

Congrats on your first Julia PR, @jebej! Welcome to julia, and thanks for taking a whack at this 🐛 .

tkelman
tkelman reviewed May 29, 2017
View reviewed changes
base/sparse/sparsematrix.jl
@@ -1428,7 +1433,7 @@ end

speye_scaled(diag, m::Integer, n::Integer) = speye_scaled(typeof(diag), diag, m, n)

function speye_scaled(T, diag, m::Integer, n::Integer)
function speye_scaled(::Type{T}, diag, m::Integer, n::Integer) where T
Copy link
Contributor

Choose a reason for hiding this comment

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

this is technically more restrictive than the existing signature, but looks like no one is calling this in registered packages and all the call sites in base pass a type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given T is passed as a type argument to Vector it needs to be a type, no?

@jebej
Copy link
Contributor Author

jebej commented May 29, 2017

Is there anything else to do here?

@KristofferC
Copy link
Member

Let's run the benchmarks just to check and then we can merge

@nanosoldier runbenchmarks("sparse", vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels

@KristofferC KristofferC merged commit 7b22adb into JuliaLang:master May 29, 2017
@KristofferC
Copy link
Member

Thanks!

@jebej jebej deleted the patch-1 branch May 29, 2017 21:06
tkelman pushed a commit that referenced this pull request Jun 3, 2017
@jebej @tkelman
Fix sparse type parameter (fixes #22110) (#22111)
88bf391 
* Fix type sparse type parameter (fixes #22110)

* No brackets for singular type param

(cherry picked from commit 7b22adb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman left review comments

@musm musm musm left review comments

@KristofferC KristofferC KristofferC left review comments

@nalimilan nalimilan nalimilan left review comments

@ViralBShah ViralBShah ViralBShah left review comments

Assignees
No one assigned
Labels
sparse Sparse arrays types and dispatch Types, subtyping and method dispatch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jebej @kshyatt @KristofferC @nanosoldier @ViralBShah @nalimilan @musm @tkelman