add Origin to specify the origin of output OffsetArray

[johnnychen94]

closes #132

[codecov]

Codecov Report

Merging #147 into master will decrease coverage by 0.67%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
- Coverage   94.44%   93.77%   -0.68%     
==========================================
  Files           3        3              
  Lines         234      257      +23     
==========================================
+ Hits          221      241      +20     
- Misses         13       16       +3     

Impacted Files	Coverage Δ
src/OffsetArrays.jl	94.20% <100.00%> (-1.20%)	⬇️
src/origin.jl	100.00% <100.00%> (ø)
src/utils.jl

---

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 a78ee17...35622c2. Read the comment docs.

[jishnub]

This looks great! I'm just wondering if the name OffsetOrigin is a bit confusing, as it seems to indicate that the origin of the array is being offset by a certain amount, however what it really does is to relocate the origin to the specified CartesianIndex.

[johnnychen94]

The ambiguity might come from the fact that "offset" can also be a verb. I originally thought it was okay because OffsetOrigin is a type and not a function.

I mean, OffsetOrigin(a, (0, 0)) can be confusing. But OffsetArray(a, OffsetOrigin(0, 0)) looks fine to me.

I'm not a native speaker 😂 I'm really bad at naming. Do you have any suggestions?

[jishnub]

I'm not a native speaker either 😆 perhaps use just Origin that is unambiguously a noun? Is this clashing with any other package?

[johnnychen94]

Is this clashing with any other package?

That's my main consideration for the Offset prefix... To clearly mark it as a name related toOffsetArrays.

[jishnub]

I guess this is fine. We may add some keyword-based constructors to simplify the process. Eg.

julia> OffsetArray(zeros(2,2), offsets = (2,2))
2×2 OffsetArray(::Array{Float64,2}, 3:4, 3:4) with eltype Float64 with indices 3:4×3:4:
 0.0  0.0
 0.0  0.0

julia> OffsetArray(zeros(2,2), origin = (2,2))
2×2 OffsetArray(::Array{Float64,2}, 2:3, 2:3) with eltype Float64 with indices 2:3×2:3:
 0.0  0.0
 0.0  0.0

In this case we don't even need to export the types.

[johnnychen94]

We may add some keyword-based constructors to simplify the process

Remove the OffsetArray(A, ZeroBasedIndex) usage or still keep it? I think we should only keep one of it.

[jishnub]

I would say keep ZeroBasedIndex, but as an unexported constant. The form is certainly convenient for someone starting off from python etc. We perhaps don't need OneBasedIndex.

[johnnychen94]

Sorry for the inaccurate question. Let me ask again about the API we're going to take.

I've added both APIs in 67c13a2 for review purposes, but I'm not very satisfied with it. Here are the questions:

• do we need to keep both APIs? OffsetArray(a, OffsetOrigin(-1, 2)) and OffsetArray(a; origin=(-1, 2))
  • if so, do we need to export OffsetOrigin, ZeroBasedIndexing, and OndBasedIndexing ? If not, then it would be likely that nobody will try to use this API.
  • to me OneBasedIndexing seems not any clearer than OffsetOrigin(1). Do we really need these two convenient names?
• does OffsetArray(a, origin=0) look strange to you for high dimensional a?

I feel we should only keep one of OffsetArray(a, origin=index) and OffsetArray(a, OffsetOrigin(index)). I'm now a little bit don't like the keyword version because it provides two equivalent ways to pass offsets: OffsetArray(a, indices...) and OffsetArray(a, offsets=indices).

ping @timholy for a final review as this PR adds new exports.

[jishnub]

Regarding the second point, I'll say that OffsetArray(a, origin=0) does look strange. It should be possible to use something like OffsetArray(a, origin = ntuple(x->0, ndims(a))). I'd say that it's reasonable to expect the value passed to origin to be a NTuple{ndims(a),Int}.

Regarding whether to add the keyword-based constructor at all, this is something that I don't have a strong opinion about, but I feel that the keyword-based versions does make it easier to understand what's going on without necessarily being familiar with the package. However this is a matter of taste, and given that the positional-argument version has been standardized maybe we can stick to that. In this case we'll perhaps have to export the types.

On that note, I actually don't mind these not being exported. Something like this seems pretty clear to me:

julia> OffsetArray(a, OffsetArrays.Origin(0,0))

This also sidesteps the issue of having to prepend Offset to the name of the type to denote the association.

I'm not sure what constitutes the API: exported types or documented types? If this usage is documented, would it still not be used if the type is not exported?

Regarding OneBasedIndexing -- do we need this at all?

[johnnychen94]

OffsetArrays.Origin(0,0) looks good to me. I'll revert 67c13a2 and stick to the positional version.

Edit: I'm now satisfied with the version in 57c7aa8. 😆

Regarding OneBasedIndexing -- do we need this at all?

I feel it awkward to only define ZeroBasedIndexing given that the major part of Julia lives in the OneBasedIndexing world 😂

Perhaps not define both of them in this PR, and try OffsetArrays.Origin for a while and see if we really want that.

I'm not sure what constitutes the API: exported types or documented types? If this usage is documented, would it still not be used if the type is not exported?

• Exported names definitely belong to the APIs. It is very hard to deprecate/remove exported names, so it should be considered the stablest one.
• undocumented internal names should not be considered stable. More often, we would explicitly tag internal-only functions with leading _(s) on its name. Speaking of this, it is more clear that splitCartesianIndices is an internal helper function if we rename it to _splitCartesianIndices.
• Whether documented non-export names are part of the API specifications is more often a personal taste. There are cases that we have good reason to not export them, for example, ImageFiltering.Kernel.* is not exported, but that's a part of the API.

OffsetArrays.Origin should not be exported because it is less clear what it means without the module namespace. But it should be a part of the API, just like ImageFiltering.Kernel.

[johnnychen94]

The nightly test failure on macOS is not related; I've verified it locally.