make performancetips code consistent with offset arrays #23565
Conversation
|
Thanks! LGTM. Anything that specifies |
doc/src/manual/performance-tips.md
Outdated
| n = size(x, 1) | ||
| out = Array{T}(n, n) | ||
| for i = 1:n | ||
| n = indices(x, 1) |
There was a problem hiding this comment.
Maybe change the name of the variable to e.g. inds.
There was a problem hiding this comment.
Yes, good point. I made the change.
doc/src/manual/performance-tips.md
Outdated
| @fastmath @inbounds @simd for i in 2:n-1 | ||
| inds = linearindices(u) | ||
| dx = 1.0 / (length(inds)-1) | ||
| @fastmath @inbounds du[inds[1]] = (u[inds[2]] - u[inds[1]]) / dx |
There was a problem hiding this comment.
In most cases the more general style is at least as clear - but in this one case it isn't, and I'm unsure whether this might confuse readers? I tried to be as clear as possible.
|
I think this might just detract from the point of the examples. I rather just have a |
|
I think that could be true for the line I highlighted above - that's why I highlighted it. I was thinking to suggest annotating that with ::Array. In the other examples I think the style is at least as clear and might be said to be a preferred style anyway, at least it does not detract from clarity IMHO. I'd say it is an issue that the very first example for inbounds here https://docs.julialang.org/en/latest/manual/performance-tips/#man-performance-annotations-1 is exactly a case that this part https://docs.julialang.org/en/latest/devdocs/offset-arrays/#Background-1 highlights as unsafe. In the end I guess it depends how much you'd like the generalized array interface to permeate the language and ecosystem. We're now editing Plots to conform with the generalized syntax and it does require a slightly different thinking. |
|
In places where it's awkward, I'm fine with restricting it to Arrays. But overall I agree that we should demo code that uses the generic interface. When I get some time again I plan to "take off the training wheels" and submit a PR to OffsetArrays that allows |
Replaces generalized indexing syntax with an ::Vector assertion where that improved clarity of the code.
|
I've 1. inserted an ::Vector assertion in the place where the code was awkward, and 2. mentioned the concern with unconventional indexing when using inbounds. This should meet the concerns. |
doc/src/manual/performance-tips.md
Outdated
| @@ -1094,6 +1094,10 @@ Sometimes you can enable better optimization by promising certain program proper | |||
| * Write `@simd` in front of `for` loops that are amenable to vectorization. **This feature is experimental** | |||
| and could change or disappear in future versions of Julia. | |||
|
|
|||
| The common ideom of using 1:n to index into an AbstractArray is not safe if the Array uses unconventional indexing, | |||
doc/src/manual/performance-tips.md
Outdated
| @@ -601,7 +601,7 @@ This should be written as: | |||
|
|
|||
| ```jldoctest | |||
| julia> function fill_twos!(a) | |||
| for i=1:length(a) | |||
| for i=linearindices(a) | |||
There was a problem hiding this comment.
Would it make more sense to use eachindex(a)?
There was a problem hiding this comment.
agreed that eachindex is more intuitive. But the offset-arrays doc recommend linearindices (https://docs.julialang.org/en/latest/devdocs/offset-arrays) so I chose that for consistency.
Should I
- keep
linearindicesboth places for consistency - change
linearindices->eachindexboth places for consistency (for this use - iterating linearly over a container) - change
linearindices->eachindexhere and not worry about the small discrepancy
?
I have no clear preference.
There was a problem hiding this comment.
Use linearindices if you really want to index with an integer. If you don't care what you index with, use eachindex.
There was a problem hiding this comment.
I see what you mean about it being ambiguous in the devdocs.
There was a problem hiding this comment.
I've updated to eachindex here. I don't mind including some changes to the devdocs here, or we could leave as it is.
There was a problem hiding this comment.
If you're willing I'd be grateful, but feel free to say "your job, Tim." 😄
There was a problem hiding this comment.
I've tried here: #23731 . Can't say I'm completely happy with the result at this point, but possibly after some feedback that could work.
|
Thanks so much, @mkborregaard! |
As remarked on Discourse (https://discourse.julialang.org/t/is-inbounds-really-necessary/5654/12) the performance tips code for inbounds is not consistent with recommended style.
The performance tips e.g. use 1:n to iterate over an array to demonstrate a case where it is safe to use
@inbounds. But that very example is used here: https://docs.julialang.org/en/latest/devdocs/offset-arrays/#Background-1 to describe a situation where it is NOT safe to use@inboundsbecause the array could be an offsetarray.When used as in the context here it is still safe, because the array is initialized in the previous line, but it is possibly misleading.
I updated the code examples on the page to be consistent with the generalized-array syntax as I understand it.