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

Return an AbstractFill from view? #84

Closed
tkf opened this issue Jan 22, 2020 · 4 comments
Closed

Return an AbstractFill from view? #84

tkf opened this issue Jan 22, 2020 · 4 comments

Comments

@tkf
Copy link
Member

tkf commented Jan 22, 2020

As it is planned to return an AbstractRange from view(::AbstractRange, ...) JuliaLang/julia#26872, I think similarly it makes sense to return an AbstractFill from view(::AbstractFill, ...). I think it is also compatible with reshape(::AbstractFill, ...) :: AbstractFill. What do you think?

@dlfivefifty
Copy link
Member

Ha I came across this issue and was planning to propose this change. I think this is a great idea.

@dlfivefifty
Copy link
Member

I worked around this in LazyArrays.jl: https://github.com/JuliaArrays/LazyArrays.jl/blob/42823c66a0344f61fafab1da96db5260079afd36/src/lazybroadcasting.jl#L213

But still preferable to overload view as you suggest.

@dlfivefifty
Copy link
Member

Ha, just deleted a long post why this is a bad idea because range doesn't do it when I noticed range does!

julia> view(1:5, 1:3)
1:3

That said, #125 makes me hesitant that downstream packages will assume that view returns a SubArray, or at the very least something that satisfies the implicit subarray interface, e.g.

v == parent(v)[parentindices(v)...]

In fact the docs for view hint strongly that this is required:

Like getindex, but returns a view into the parent array A with the given
indices instead of making a copy
Since this is inconsistent with the behaviour of ranges I'm going to make a PR to alter the docs for view.

dlfivefifty added a commit that referenced this issue Dec 10, 2020
dlfivefifty added a commit that referenced this issue Jan 6, 2021
* view returns a Fill (#84)

* add tests

* size -> axes in broadcasted
@dlfivefifty
Copy link
Member

This has been implemented.

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

2 participants