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 #19503 (provide a unary minus method specialized for sparse matrices) #19530

Merged
merged 1 commit into from
Dec 14, 2016

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Dec 7, 2016

As @StephenVavasis caught in #19503 (thanks!), I accidentally removed the unary minus method specialized for sparse matrices in #17265. This pull request provides a replacement method. Fixes #19503. Best!

@StefanKarpinski
Copy link
Member

Is there any way to test that we don't accidentally delete this again in the future?

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 7, 2016

Is there any way to test that we don't accidentally delete this again in the future?

I was wondering the same. In an earlier PR I somehow checked calling of a specialized method, but I haven't been able to figure out which PR that was. Will spend a bit more time looking. Thanks for reviewing! Edit: Found it, #16979. Update inbound.

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 7, 2016

Now with a test. Thanks!

@kshyatt kshyatt added performance Must go faster sparse Sparse arrays labels Dec 7, 2016
@simonbyrne
Copy link
Contributor

Could also add something to BaseBenchmarks.jl?

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 8, 2016

Could also add something to BaseBenchmarks.jl?

Could, but not certain that's worthwhile? The test included in this PR should prevent regressions, and a benchmark of the one-liner in this PR distills to a benchmark of map(-, <:Vector) and copy(<:Vector). Thoughts? Thanks!

@tkelman
Copy link
Contributor

tkelman commented Dec 8, 2016

always worth adding more benchmarks

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 8, 2016

Cheers, I can prepare a BaseBenchmarks PR at some point if you feel this is worth benchmarking. Question though:

always worth adding more benchmarks

Does that statement not fail at some point? Consider this illustrative hyperbole: Would a benchmark equivalent to @benchmark (x::Int = 0; for k in 1:typemax(Int); x += 1; end) be worthwhile? And with such a benchmark, would an additional benchmark @benchmark (x::Int = 1; for k in 2:typemax(Int); x += 1; end) be worthwhile? Where does one draw the line?

@tkelman
Copy link
Contributor

tkelman commented Dec 8, 2016

If it's completely redundant with existing benchmarks then it's probably not worth adding. But I don't think this is, since the tests and dispatch here can change over time and independently tracking the performance of the end-user operation that was reported here is worthwhile.

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 8, 2016

If it's completely redundant with existing benchmarks then it's probably not worth adding. But I don't think this is, since the tests and dispatch here can change over time and independently tracking the performance of the end-user operation that was reported here is worthwhile.

Cheers, I appreciate the explanation!

@Sacha0
Copy link
Member Author

Sacha0 commented Dec 13, 2016

Suggested benchmark submitted: JuliaCI/BaseBenchmarks.jl#45. Thanks!

@tkelman tkelman merged commit bfc72b6 into JuliaLang:master Dec 14, 2016
@Sacha0 Sacha0 deleted the spunaryminus branch December 14, 2016 23:40
@Sacha0
Copy link
Member Author

Sacha0 commented Dec 14, 2016

Thanks for reviewing / merging!

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

Successfully merging this pull request may close these issues.

5 participants