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

Propagate AxisArray copy / view down to taking copies / views of its axes as well #148

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kcajf
Copy link

@kcajf kcajf commented Sep 27, 2018

This is my attempt at addressing #147

All the tests appear to pass. I'm new to Julia and wasn't quite sure on the best way implement this, but doing it via Value types seemed efficient and easy. The only issue was that, in order to resolve the following method ambiguity:

  LoadError: MethodError: AxisArrays._new_axes(::Axis{:y,Base.OneTo{Int64}}, ::Val{false}, ::AxisArray{Int64,1,Base.OneTo{Int64},Tuple{Axis{:y,Base.OneTo{Int64}}}}) is ambiguous. Candidates:
    _new_axes(ax::Axis{name,T} where T, copy::Val, idx::AxisArray{#s19,N,D,Ax} where Ax where D where #s19) where {name, N} in AxisArrays at /data/home/jfrigaa/AxisArrays.jl/src/indexing.jl:96
    _new_axes(ax::Axis{name,T} where T, copy::Val{false}, idx::AbstractArray{T,1} where T) where name in AxisArrays at /data/home/jfrigaa/AxisArrays.jl/src/indexing.jl:81

I had to duplicate _new_axes on line 95 with the copy parameter as both Val{true} and Val{false} (whereas I originally just had the argument as copy::Val). I couldn't work out another way to get it to compile, but no doubt someone will be able to suggest a better way to avoid this ambiguity without duplicating code.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

An alternative to just-plain-Val would be to create a CopyStyle trait. However, since this is an internal interface I think it's fine to use just-plain-Val.

src/indexing.jl Outdated Show resolved Hide resolved
src/indexing.jl Outdated Show resolved Hide resolved
src/indexing.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Member

timholy commented Dec 12, 2018

Didn't notice how long ago this came. It's been hard for folks to find time to review PRs here. Sorry for the delay.

@kcajf
Copy link
Author

kcajf commented Dec 24, 2018

Ended up adding a CopyStyle trait for safety to avoid the case of e.g. passing copy=Val(123) and hitting an unspecialized method rather than erroring.

@kcajf
Copy link
Author

kcajf commented Jun 21, 2019

Shall we merge this?

@bgroenks96
Copy link

Uh hey, yeah, this would be really nice to have. I assume this is the reason that AxisArrays cause allocation to occur when taking subarrays, e.g toy example:

function testalloc(x,interval)
    @view x[interval];
    nothing
end

causes allocation when x is an AxisArray but not when it's a normal array. This is true regardless of whether interval is a Range or an array of indices.

@bgroenks96
Copy link

I'll create a separate issue for this and then link to this PR.

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.

4 participants