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

Create @view macro for creating SubArrays via indexing. #16564

Merged
merged 1 commit into from
Jun 27, 2016

Conversation

simonbyrne
Copy link
Contributor

@simonbyrne simonbyrne commented May 24, 2016

Allows writing things like

@sub A[1,2:end]

In order to replicate current indexing behaviour, this actually calls slice.

I'm fairly confident this replicates the end replacing behaviour in

julia/src/julia-syntax.scm

Lines 93 to 123 in c43b5e5

;; return the appropriate computation for an `end` symbol for indexing
;; the array `a` in the `n`th index.
;; `tuples` are a list of the splatted arguments that precede index `n`
;; `last` = is this last index?
;; returns a call to endof(a), trailingsize(a,n), or size(a,n)
(define (end-val a n tuples last)
(if (null? tuples)
(if last
(if (= n 1)
`(call (top endof) ,a)
`(call (top trailingsize) ,a ,n))
`(call (top size) ,a ,n))
(let ((dimno `(call (top +) ,(- n (length tuples))
,.(map (lambda (t) `(call (top length) ,t))
tuples))))
(if last
`(call (top trailingsize) ,a ,dimno)
`(call (top size) ,a ,dimno)))))
;; replace `end` for the closest ref expression, so doesn't go inside nested refs
(define (replace-end ex a n tuples last)
(cond ((eq? ex 'end) (end-val a n tuples last))
((or (atom? ex) (quoted? ex)) ex)
((eq? (car ex) 'ref)
;; inside ref only replace within the first argument
(list* 'ref (replace-end (cadr ex) a n tuples last)
(cddr ex)))
(else
(cons (car ex)
(map (lambda (x) (replace-end x a n tuples last))
(cdr ex))))))

This was discussed in an issue at some point, but I forgot which one.

end

"""
@sub A[...]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be added to rst docs somewhere if exported

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I do that? I couldn't figure it out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's somewhat documented in CONTRIBUTING.md I think. The gist of it is pick a place for it in the rst, add just the signature, then make docs should fill in the rest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@tkelman
Copy link
Contributor

tkelman commented May 25, 2016

Doing this as a macro means it can't support things like A[1:end-1], can it?

hm what was it about end that needed to be handled by lowering then?

@mbauman
Copy link
Member

mbauman commented May 25, 2016

Nice! Some previous discussion is buried in the Arraypocalype issue and starts here: #13157 (comment)

One question is if the macro should recurse through all non-assignment refs or not. I think I'd prefer the recursive behavior, allowing for @sub begin #= algorithm #= end to easily use views everywhere.

@simonbyrne
Copy link
Contributor Author

Recursive behaviour is more complicated, and we would need to figure out some possible ambiguities like:

@sub A[B[2:end]]

Should that create subarrays of both A and B?

I figured this was a reasonable start, and recursive behaviour could be added later.

@simonbyrne
Copy link
Contributor Author

hm what was it about end that needed to be handled by lowering then?

I'm not sure, but this might require input from someone who knows more about the internals.

@tkelman
Copy link
Contributor

tkelman commented May 25, 2016

I think I was just misreading the caveats about the macro option from the long discussion, it was warned that you'd have to reimplement the end lowering behavior in the macro which is exactly what you've done here. More complicated than just replacing getindex with slice, but not too many lines of code overall. One step closer to having a pure-Julia implementation of all lowering?

"""
@sub A[inds...]

Creates `SubArray` from an indexing expression. This can only be applied directly to a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Creates a SubArray"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch.

@cstjean
Copy link
Contributor

cstjean commented May 25, 2016

#15750 is relevant: use first/last as endpoints for Range indexing

@timholy
Copy link
Member

timholy commented May 25, 2016

I'm nervous about #15750 because I worry it can't handle all situations. This is certainly safer.

Hopefully in the not too distant future we'll also have to generalize this for #16260, and decide whether to add support for begin.

@timholy
Copy link
Member

timholy commented May 25, 2016

hm what was it about end that needed to be handled by lowering then?

The point has always been that we couldn't handle sub(A, 2:end) because the lowering looks for the containing Expr(:ref, ...), and when you use end as an argument inside a function there isn't such a containing expression. In this case, the macro gets to work with such an expression, so all is well.

I should also point out that Jeff has made a related proposal, adding A@[2:end] to create a view. On balance, though, I think I prefer @sub, mainly because someday I will probably want to add @index and @stored too (see ArrayIteration.jl).

@simonbyrne
Copy link
Contributor Author

I should also point out that Jeff has made a related proposal, adding A@[2:end] to create a view. On balance, though, I think I prefer @sub, mainly because someday I will probably want to add @index and @stored too (see ArrayIteration.jl).

The thought just occurred to me that we could use A@sub[2:end], which would simply be syntactic sugar for @sub(A[...])? This would then encompass your other examples.

@tkelman
Copy link
Contributor

tkelman commented Jun 2, 2016

There's some sentiment in #13157 that this might be better named @view, ArrayViews use can be qualified in packages.

@simonbyrne
Copy link
Contributor Author

I originally proposed it as @view, but I thought @sub might be better since, (a) it's 1 character shorter, and (b) it calls sub (okay, it actually calls slice, but I presume we're changing that?).

@simonbyrne
Copy link
Contributor Author

simonbyrne commented Jun 2, 2016

Also worth providing a link to the original inspiration JuliaArrays/ArrayViews.jl#14 (which I had forgotten).

@mbauman mbauman mentioned this pull request Jun 9, 2016
27 tasks
@simonbyrne simonbyrne changed the title Create @sub macro for creating SubArrays via indexing. Create @view macro for creating SubArrays via indexing. Jun 19, 2016
@simonbyrne
Copy link
Contributor Author

I've renamed to @view to mirror #16972. I've also changed it so that it expands to true && view(X, inds...), to prevent people redefining view by doing @view(X[inds...]) = ...

@tkelman
Copy link
Contributor

tkelman commented Jun 26, 2016

Merge?

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

Successfully merging this pull request may close these issues.

5 participants