Use sort! where possible #19973
Use sort! where possible #19973
Conversation
|
Would it be worth checking Nanosoldier here or is that pretty unnecessary? |
| @@ -46,7 +46,7 @@ function sorted_array(m::Dict{AbstractString, Int}) | |||
| kn[i] = KNuc(elem...) | |||
| i += 1 | |||
| end | |||
| sort(kn) | |||
| sort!(kn) | |||
There was a problem hiding this comment.
Since this code is used to track performance, it's probably better to leave this change out, to maintain continuity. (I doubt it's that important, though, so I'm open to arguments otherwise...)
There was a problem hiding this comment.
Yeah, I guess in this case is better to leave this out.
|
@ararslan I don't think there's need to ask |
| @@ -46,7 +46,7 @@ function sorted_array(m::Dict{AbstractString, Int}) | |||
| kn[i] = KNuc(elem...) | |||
| i += 1 | |||
| end | |||
| sort!(kn) | |||
| sort(kn) | |||
There was a problem hiding this comment.
I have to say, I don't understand why using sort! here is not ok. We use f.() in the Laplace benchmark now, how is using inplace APIs where available any different than that?
There was a problem hiding this comment.
It's fine to use sort! here. It just means that there'll be a small bump in performance which is only due to changes in the benchmark, not changes in Julia. If one were interested in, say, tracking Julia over time (e.g., like speed.julialang.org used to), this might be important.
We use
f.()in the Laplace benchmark now, how is using inplace APIs where available any different than that?
This syntax wasn't previously available in Julia, so the (presumed) speedup is precisely because of the syntax change.
Anyway, I am (and was) nitpicking. In some circumstances, benchmark changes really should be avoided, but for Julia, it should be easy to point to the blip in the k_nucleotide timings and note that it was caused by a change in the test.
And if it's more important to compare with other languages, by all means, this should use sort!.
Cheers!
I noticed here that there are a few places where we are using
sortinstead ofsort!. There are just a handful of them and they are not performance-critical, but the changes were easy enough that I decided to do them anyway.