Best practice on properly supporting OffsetArrays

[johnnychen94]

This package is catching the eyes of the entire community, let's write some best practices on this so that people know how to handle it appropriately.

closes #282

cc: @t-bltg

[codecov]

Codecov Report

Merging #281 (9be698b) into master (d8c8d42) will not change coverage.
The diff coverage is n/a.

❗ Current head 9be698b differs from pull request most recent head e9fd3f0. Consider uploading reports for the commit e9fd3f0 to get more accurate results

@@           Coverage Diff           @@
##           master     #281   +/-   ##
=======================================
  Coverage   95.90%   95.90%           
=======================================
  Files           5        5           
  Lines         464      464           
=======================================
  Hits          445      445           
  Misses         19       19           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8c8d42...e9fd3f0. Read the comment docs.

[jishnub]

This seems a bit vague, and I'm not certain that I fully understand the cases where it might not work. Maybe we can suggest OffsetArrays.no_offset_view if one requires 1-indexed arrays in their own code, and Base.require_one_based_indexing if they require users to pass 1-indexed arrays as arguments?

[johnnychen94]

Sorry. I was thinking of the A * B example for offset arrays -- for this we don't have a well-defined behavior on what axes should be used or what axes should be output, in this case, it's better to just use Base.require_one_based_indexing and leave it to users to do the conversion.

[ChrisJefferson]

(coming in as a julia newbie), maybe something like (this text is probably awful, feel free to edit / ignore)

Only use @inbounds if profiling shows it is worthwhile

@inbounds allows code to turn off bounds checking. It's easy to use incorrectly, particularly when OffsetArrays are used -- even the official example of its use in Julia's documentation was wrong for a while. Avoid @inbounds unless you have benchmarked your code and found it is useful -- as Julia's compiler gets better the cost of bounds checking usually small, or even 0, and using @inbounds changes bugs from throwing an assertion to silently returning invalid values.

[johnnychen94]

@ChrisJefferson this is a useful comment from my experience!

The @inbounds macro only becomes useful when it happens in the innermost loop -- it is usually a trick to eliminate the unnecessary boundcheck if-branch and thus enable CPU SIMD and make codes much faster.
For any non-trivial functions, @inbounds can become quite useless speaking of performance improvement. This means that @inbounds checks are often unnecessary for common package users.

But I'm afraid the most appropriate place to put this comment is somewhere in the Julia documentation, I'll try to find out a place to add this and submit a PR to the upstream Julia repo. But let me share this to JuliaLang/julia#45342 first.

[PallHaraldsson]

Isn't it best to merge this in whole (or in part? at least the pointer on --check-bounds=yes that closes #282).

I read the text and it seems fine by me (I'm not the expert). My proposal #284 (trying to avoid the issue) was closed. I may need to think it more carefully, until then having best (or better) practice is good.

Please do not delay too much merging some "best" practice. Perfect is the enemy of the good. The text can always be improved later.

[timholy]

There's also https://docs.julialang.org/en/v1/devdocs/offset-arrays/ which goes into more detail. Should we link that from here? nvm, I see it is. Thanks!!