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

Arraypocalypse slicing compatibility #158

Closed
c42f opened this issue Dec 26, 2015 · 15 comments
Closed

Arraypocalypse slicing compatibility #158

c42f opened this issue Dec 26, 2015 · 15 comments

Comments

@c42f
Copy link
Member

c42f commented Dec 26, 2015

Not a lot has happened over at JuliaLang/LinearAlgebra.jl#255 yet, but the breakage from APL-style slicing (JuliaLang/julia#13612) is already apparent. I hope there's something that can be done in Compat to ease the slicing transition. Here's one possibility:

  1. For 0.4, @compat A[:,1] rewrites the :ref AST node to include a placeholder type which can be dispatched on - something like A[SliceAPL, :, 1].
  2. We then implement special versions of getindex and setindex! taking a SliceAPL as the first index, which inspect the types of the incoming slice expression and squeeze out any singleton dimensions as required.
@c42f
Copy link
Member Author

c42f commented Dec 26, 2015

Ugh, the above approach doesn't work cleanly due to a raft of ambiguities when defining the appropriate getindex. Might be better to wrap the array itself in a new array wrapper type, ie A[:,1] rewrites to ArrayAPL(A)[:,1]. Then define methods on the ArrayAPL type. This would probably generalize to some of the other anticipated breakage.

@martinholters
Copy link
Member

Are you (or is anyone else) working on this? The occasions where I think this would come handy seem to become more often, lately...

@c42f
Copy link
Member Author

c42f commented Mar 22, 2016

No, I'm afraid I haven't had time to look at it. I've been stuck on 0.4 lately.

@andreasnoack
Copy link
Member

Is it the fact that A[1,:] returns a Vector that is an issue here? If so then remember that A[1:1,:] and vec(A[1,:]) return the same types on 0.4 and 0.5.

@martinholters
Copy link
Member

Right, and vec(A[1,:]) is even shorter than @compat A[1,:]. But is there a similarly nice construct for more dimensions?

Oh, and beyond the scope of this issue maybe, but what about sparse matrices?

@c42f
Copy link
Member Author

c42f commented Mar 23, 2016

@andreasnoack good point. It's not an ideal solution though - it litters package code with constructs which are not obviously related to backward compatibility.

@andreasnoack
Copy link
Member

But is there a similarly nice construct for more dimensions?

I cannot come up with anything nice right now. I guess that squeeze(1, A[1:1,:,:]) would work but that is not nice.

@c42f I'm fine with Compat support. I just wanted to point out that a solution existed.

@timholy
Copy link
Member

timholy commented Mar 23, 2016

The best approach would be to "backport" the getindex changes to Compat, using a different name and having @compat call the new name. Might not be a small job, however.

@c42f
Copy link
Member Author

c42f commented Mar 23, 2016

@timholy I guess this would mean the @compat macro would need to understand the lowering from :ref AST nodes to getindex/setindex? Does that present problems?

@timholy
Copy link
Member

timholy commented Mar 23, 2016

I think it would be manageable, but probably the challenges will be clear only by trying it. I would think about adding a global variable is_lhs ("is left-hand-side") and have @compat set it/check it when parsing Expr(:(=), args...).

@martinholters
Copy link
Member

I've tried @c42f's idea of rewriting A[...] to ArrayAPL(A)[...] and then providing getindex for that - looks like it might be working. I'll open a PR for discussion soonish.

@andreasnoack
Copy link
Member

Is this still relevant?

@martinholters
Copy link
Member

Theoretically, yes. We still support Julia 0.4 which differs from Julia 0.5 and later in this respect and have no @compat for it. Practically, it looks like no-one is missing it, given how dormant this thread is.

I'd say we keep this open until we bump minimum Julia version to 0.5, close it then (along with JuliaLang/julia#179).

@c42f
Copy link
Member Author

c42f commented Jan 19, 2017

In practice I haven't found this to be a major pain point in packages I maintain. It's a bit of an annoyance on occasion, but working around it manually isn't too hard.

@martinholters
Copy link
Member

I'd say we keep this open until we bump minimum Julia version to 0.5, close it then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants