Indexing ComMatrix

[kescobo]

This came up a couple of times as I was trying to revamp Microbiome.jl - the getindex() function defined here doesn't seem to work, or at least doesn't work as I expected it to:

julia> com = ComMatrix(rand(10,2), string.(collect(1:10)), string.(collect(1:2)))
ComMatrix with 10 species in 2 sites

Species names:
1, 2, 3...9, 10

Site names:
1, 2


julia> com[1,2]
ERROR: MethodError: Cannot `convert` an object of type Float64 to an object of type SpatialEcology.ComMatrix
This may have arisen from a call to the constructor SpatialEcology.ComMatrix(...),
since type constructors fall back to convert methods.
Stacktrace:
 [1] getindex(::SpatialEcology.ComMatrix{Float64}, ::Int64, ::Int64) at /Users/kev/.julia/v0.6/SpatialEcology/src/Commatrix_functions.jl:108
 [2] eval(::Module, ::Any) at ./boot.jl:235
 [3] eval(::Any) at ./boot.jl:234
 [4] macro expansion at /Users/kev/.julia/v0.6/Atom/src/repl.jl:118 [inlined]
 [5] anonymous at ./<missing>:?

This also seems to break setindex!() as far as I can tell.

[mkborregaard]

Oh, thanks, yes I think i abandoned those over using view but maybe that's not the best approach.

[mkborregaard]

I see a number of ways to change this, but let me ask you: what would you expect to get from com[1:3,1]? A 3x1 Matrix{Float64}? or a ComMatrix with 3 species and 1 site? I think from com[1,2] most people would expect to just get a scalar Float64 like indexing into an Array (which you don't get now, but I think you should).
I think @view com[1:3,1] should always return a SubComMatrix{T}with 3 species and 1 site, but that @view com[1,2] should also just return the scalar.
Any thoughts on this before I start fixing? (cc @richardreeve)

[kescobo]

what would you expect to get from com[1:3,1]? A 3x1 Matrix{Float64}? or a ComMatrix with 3 species and 1 site?

To be honest, I hadn't considered the case of indexing a range. I can definitely see this going either way, though I think retaining the ComMatrix structure in this case would be more intuitive. When I built my version, I just had it index the underlying matrix, since that was easier, and I'd only considered the case of grabbing a single value. In that case, it seems like simply getting the value back (rather than a 1x1 ComMatrix) makes more sense, though it may be that these two positions are incompatible.

[richardreeve]

I see a number of ways to change this, but let me ask you: what would you expect to get from com[1:3,1]? A 3x1 Matrix{Float64}? or a ComMatrix with 3 species and 1 site?

I certainly agree about the com[1,2] and @view com[1:3,1] cases, but I'm less certain what I'd expect from @view com[1,2]. My understanding of @view is that it should always return a sub-array (from the docs). And indeed:

julia> a = [1 2; 3 4]
2×2 Array{Int64,2}:
 1  2
 3  4

julia> @view a[1,2]
0-dimensional SubArray{Int64,0,Array{Int64,2},Tuple{Int64,Int64},false}:
2

does return an array. I don't feel strongly about com[1:3,1] at the moment - I guess if anything I feel a bit like this is a low-level interface that should return something suitably low-level, like a Matrix{Float64}, but as I say, I don't feel strongly about it.

[kescobo]

I like the idea of having straight indexing return the underlying value/matrix, while @view returns a ComMatrix. This seems quite sensible to me.

[tpoisot]

My preference would be that if we have some way of indexing returning another ComMatrix, then all indexing should do it too.
But if the goal is to extract the content at one index in the matrix, maybe we need another function.

[mkborregaard]

Thanks guys, that's helpful. After thinking about it I think it makes the most sense to have ComMatrix behave like a regular Array (which involves dropping dimensions, in this case that means returning a scalar when indexing for a single value but otherwise returning a ComMatrix). It doesn't currently inherit from AbstractArray, but that may change in the future if we redefine ComMatrix as a particular parametrization of an AxisArray as discussed here EcoJulia/Diversity.jl#23 (comment)

I'll make the PR here.