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

slices of matrices should be vectors #231

Closed
StefanKarpinski opened this issue Oct 17, 2011 · 20 comments
Closed

slices of matrices should be vectors #231

StefanKarpinski opened this issue Oct 17, 2011 · 20 comments
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior needs decision A decision on this change is needed

Comments

@StefanKarpinski
Copy link
Member

julia> X = [ i^2 - j | i=1:10, j=1:10 ];

julia> typeof(X)
Array{Int64,2}

julia> X[:,1]
10x1 Int64 Array
0 
3 
8 
15 
24 
35 
48 
63 
80 
99 
@ghost ghost assigned ViralBShah Oct 17, 2011
@JeffBezanson
Copy link
Member

This is fairly significant. Is this the rule we originally wanted way back when: "the rank of the result is the sum of the ranks of the indexes"? Or is it squeezing singleton dimensions? Or just trimming trailing singleton dimensions?
Interestingly we have this for SubArray. sub is the same as our current ref, and slice removes dimensions indexed by scalars.

@StefanKarpinski
Copy link
Member Author

As in it's a significant question as to how this should behave, or that it's a significant bug?

@JeffBezanson
Copy link
Member

I think the current behavior is intentional, and settling this question is a big deal.

@StefanKarpinski
Copy link
Member Author

Yes, that's why I assigned this to Viral — I think it's intentional too. I don't care for it, however, but I guess let's have that discussion again now.

@ViralBShah
Copy link
Member

We need a consistent rule. Usually, when you do that, you do want a vector.

-viral

On Oct 17, 2011, at 6:53 PM, Stefan Karpinski wrote:

Yes, that's why I assigned this to Viral — I think it's intentional too. I don't care for it, however, but I guess let's have that discussion again now.

Reply to this email directly or view it on GitHub:
#231 (comment)

@JeffBezanson
Copy link
Member

For X[:,1] a vector seems right, but do you want something 2d from X[:,1,:]?

@StefanKarpinski
Copy link
Member Author

For X[:,1] a vector seems right, but do you want something 2d from X[:,1,:]?

Yes. What else would it be? Another 3-tensor? Keep in mind that if you want that, you can still do X[:,1:1,:].

@JeffBezanson
Copy link
Member

We have to beware the type inference implications of this. I'm not sure how to deal with a tuple (or integer) whose length depends on the types of a sequence of values. It seems possible, but it's probably at the outer edge of what we're capable of. We might even need to add a special feature like ranksum().

Here's how slice for SubArrays does it:

n = 0
for j = i; if !isa(j, Index); n += 1; end; end

It's not too realistic to expect to run that whole loop at compile time.

@StefanKarpinski
Copy link
Member Author

Would it help if we were stricter about indexing behaviors? As in you can only index into an array with the number of indices equal to the number of dimensions of the array. In other words, would eliminating linear indexing help?

@JeffBezanson
Copy link
Member

No, because even if you have the right number of indexes, you still need something like that rank-counting loop above.

@StefanKarpinski
Copy link
Member Author

Hmm. So what are you thinking? If slices of tensors are always the same rank as the tensor being sliced, then how do you get a vector from a matrix? Or a matrix from a 3-tensor?

@StefanKarpinski
Copy link
Member Author

I guess one approach could be that slicing always preserves the rank, making inference easy since it's completely stable. But then you need a compress function that can discard dimensions with a size of one. I'm not sure if that actually simplifies anything, however.

@JeffBezanson
Copy link
Member

We have squeeze to do that, but then it's based on singleton dimensions rather than ranks of indexes, so A[:,1:1,:] would also be 2d. So far the only thing I can think of is a special function that type inference knows computes the sum of the ranks of its arguments.

@StefanKarpinski
Copy link
Member Author

That seems like a reasonable thing to have. It's special case, but we know those are necessary to make things like this work.

@JeffBezanson
Copy link
Member

Maybe the rule we want is to drop dimensions for trailing scalars? So A[:,1] is 1-d, but A[1,:] is still 2-d? A little wonky perhaps.
The other thing we might have settled on is to keep indexing the way it is, but ignore trailing singleton dimensions when comparing shapes. And/or use promote to add trailing singleton dimensions where needed.

@StefanKarpinski
Copy link
Member Author

That might actually work but it does feel like a bit of a hack.

@pao
Copy link
Member

pao commented Mar 2, 2012

Here's another case to cover which can come up when range indexing with a variable:

julia> [1 2; 3 4][1,1]
1

julia> [1 2; 3 4][1,1:1]
1x1 Int32 Array:
 1

The problem is that a 1x1 array isn't dimensionally compatible with anything, even though it should be--it's really a scalar. There's not really a good workaround, as squeeze()ing leaves you with a 0-dimensional Array, which is even less useful.

@JeffBezanson
Copy link
Member

That probably can't change, since in general you might do A[1:m,1:n] and
expect a mxn array as the result.
On Mar 1, 2012 9:04 PM, "pao" <
reply@reply.github.com>
wrote:

Here's another case to cover which can come up when range indexing with a
variable:

julia> [1 2; 3 4][1,1]
1

julia> [1 2; 3 4][1,1:1]
1x1 Int32 Array:
 1

The problem is that a 1x1 array isn't dimensionally compatible with
anything, even though it should be--it's really a scalar. There's not
really a good workaround, as squeeze()ing leaves you with a 0-dimensional
Array, which is even less useful.


Reply to this email directly or view it on GitHub:
#231 (comment)

@pao
Copy link
Member

pao commented Mar 2, 2012

In general, yes. The situation I was working with had the end of the range set by a variable and this hit on the first iteration. On further review I needed to fix indexing on the other side of the operation--I got screwed up when converting to preallocating the other array. I still think the behavior of squeeze() is wrong here. I'll file a separate issue for that.

JeffBezanson added a commit that referenced this issue Mar 9, 2012
…n dims

inference on it is currently sub-optimal for unequal dimensions with some dimension greater than 2.
issue #231
@JeffBezanson
Copy link
Member

We now consistently treat arrays so that trailing singletons don't matter, so we should be ok here.

cmcaine added a commit to cmcaine/julia that referenced this issue Sep 24, 2020
This means markers and users don't need to fiddle with the tests file.
Keno pushed a commit that referenced this issue Oct 9, 2023
* use non rng seed dependent uuid to avoid collisions

* use our own rng
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

4 participants