-
Notifications
You must be signed in to change notification settings - Fork 40
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
[RFC] when should Colon :
keep offset information
#246
Comments
The present behaviour of making M = reshape(1.0:16, 0:3, 3:6) .+ 0.0
reshape(M, 0:7, :) # does this 2nd dimension share an identity with the 3:6 one?
reshape(M, 0:3, 1, :) # ... yet this 3rd dimension (with the same length) does not?
reshape(M, axes(M,1), :, axes(M,2)) # here I'm inserting a trivial dimension BTW TensorCast now handles offsets, and some kinds of reshaping. My 2nd and 3rd examples would be written like this, it knows that the last index of B was the last index of M, and thus it will preserve its axis: @cast B[i,_,j] := M[i,j] The closest you can get to my 1st example above is |
This might be relevant:
In this sense reshaping for arrays should always use linear indices of the parent, and we view the |
Agreed with @jishnub:
That last bit could merit a bit of thinking during actual implementation wrt what types get created. |
In a normal 1-based indexing array world, this is clear:
:
serves as a length placeholder.During #220 #228 and #245, I've realized that we haven't yet had a clear and consistent definition of the role of
:
in the OffsetArray world. Let's takereshape
as an example, it might also apply to all other operations where:
is allowed, e.g.,getindex
,setindex!
.I propose the rule of thumb is to keep offset information if it's unambiguous. It comes with one and only one extra rule: if all
inds
inputs are range type, keep offset information for the corresponding dimension where:
is placed at.All other cases should be consistent with the Base case. For example:
In this case, it's ambiguous whether
:
is used as a length placeholder or axes placeholder so we should stick to the Base case; otherwise, I can foresee a lot of type piracy involved.The text was updated successfully, but these errors were encountered: