Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Indexing by name when multiple dimensions have same name #52

Open
nickrobinson251 opened this issue Jun 25, 2019 · 4 comments
Open

Indexing by name when multiple dimensions have same name #52

nickrobinson251 opened this issue Jun 25, 2019 · 4 comments
Labels
decision Something that needs to be decided

Comments

@nickrobinson251
Copy link
Contributor

nickrobinson251 commented Jun 25, 2019

We currently consider this undefined behaviour -- and i wonder if that's an unfortunate middle ground.

In practice (using v0.2) we have consistent and reasonable behaviour: when there are duplciate names. indexing by name indexes both dimenstions

nda = NamedDimsArray{(:a, :b, :b)}(rand(10, 20, 30));
nda[a=2, b=3] == nda[2, 3, 3]
getindex(nda, b=1) == nda[:, 1, 1]

I wonder if we should commit to supporting this (and document it)?

Alternatively, we could go the other direction - perhaps we think this could become a source of subtle bugs for users - and error upon "ambiguous" indexing, i.e. with naems(nda) == (:a, :b, :b), should nda[a=2, b=3] throw an informative error?

@nickrobinson251
Copy link
Contributor Author

The more I think about this, the more I like it, and think we should document it. Although I'm still not sure I would want to actually use this behaviour for anything serious... it's a bit magic.

@mcabbott
Copy link
Collaborator

Having the same name twice seems fairly natural for d = Diagonal(NamedDimsArray(rand(3), :i)), for which d[i=1] is currently an error, but could be a number.

The other place this seems likely to occur is an outer product, like so:

julia> m = NamedDimsArray(rand(2,4), (:i, :k));

julia> p = m' * m
4×4 NamedDimsArray{(:k, :k),Float64,2,Array{Float64,2}}:

julia> p[k=3]
0.22535458930936356

Indexing that down the diagonal seems wrong to me.

One idea might be to forbid the creation of such objects, and insist that * produce NamedDimsArray{(:k, :k′), here. Would this cause other problems?

(Presumably creation by hand like NamedDimsArray(ones(2,2), (:k, :k)) should then be an error.)

@nickrobinson251
Copy link
Contributor Author

nickrobinson251 commented Aug 28, 2019

Having the same name twice seems fairly natural for d = Diagonal(NamedDimsArray(rand(3), :i)), for which d[i=1] is currently an error, but could be a number.

this is an error becuase you cannot index a Diagonal by name, only a NamedDimsArray (which you know #65, but in case other folks don't)

One idea might be to forbid the creation of such objects, and insist that * produce NamedDimsArray{(:k, :k′), here. Would this cause other problems?

It is definitely an aim of this package to support duplicate names. If that's not written down publicly yet, then it should be (either in the readme or when we add docs #30) :) e.g. we want behaviour like

julia> using Statistics, NamedDims

julia> nda = NamedDimsArray(rand(4, 2), (:x, :y));

julia> Σ = cov(nda)
2×2 NamedDimsArray{(:y, :y),Float64,2,Array{Float64,2}}:
  0.0258206   -0.00923985
 -0.00923985   0.0569803

The only question is whether we should officially support Σ[y=1] == Σ[1, 1]

@mcabbott
Copy link
Collaborator

It is definitely an aim of this package to support duplicate names.

OK. Then I would vote that p[k=3] should be an error, it seems too surprising to return a number when indexing a full matrix that accidentally has duplicate names. Although I see that Σ[y=1] makes some sense.

(Perhaps things like NamedDims{...Diagonal} should evade the error, by not really being two dimensional.)

@nickrobinson251 nickrobinson251 added the decision Something that needs to be decided label Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decision Something that needs to be decided
Projects
None yet
Development

No branches or pull requests

2 participants