-
Notifications
You must be signed in to change notification settings - Fork 42
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
Breaking: add iterate and collect #811
Conversation
The weirdest thing about this is you still splat layers into a Into a The Array/NamedTuple duality thing that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Added some comments - not sure how you intend the API to work. I should be able to push my code as it is tomorrow, that will be a better example of the usecase.
(::Type{T})(st::AbstractDimStack) where T<:AbstractDimArray = | ||
T([st[D] for D in DimIndices(st)]; dims=dims(st), metadata=metadata(st)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that Raster(RasterStack(...))
is actually an expensive operation / materializes everything in memory? Is there a way we could avoid this, maybe a ConcatDimArray
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah in this implementation. UnstackedDimArray
or InterleavedDimArray
might be a better name because its really interleaving the layers not concatenating.
I've also been thinking of just making AbstractDimStack
an AbstractArray
itself which would make that a moot point. This PR is a step in that direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now lets just make it expensive
Base.vec(st::AbstractDimStack) = vec(collect(st)) | ||
@propagate_inbounds Base.iterate(st::AbstractDimStack) = iterate(st, 1) | ||
@propagate_inbounds Base.iterate(st::AbstractDimStack, i) = | ||
i > length(st) ? nothing : (st[DimIndices(st)[i]], i + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any way to hold off on re-instantiating the DimIndices object every time? Indexing with linear indices (single integers) currently does what I want anyway, it just needs to be forwarded correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be free anyway. And if you index with linear indices it has to do DimIndices(A)[i] internally anyway!
Mixed dimension stacks indexing really only works properly now because Dimension indexing is used internally.
Base.map(f, s::AbstractDimStack) = error("Use maplayers(f, stack)) instad of map(f, stack)") | ||
Base.map(f, ::Union{AbstractDimStack,NamedTuple}, xs::Union{AbstractDimStack,NamedTuple}...) = | ||
error("Use maplayers(f, stack, args...)) instad of map(f, stack, args...)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the cost...but should it be? Arguably if you expected iterate
to work the way it will now, you also expected map
to work that way...
Is it a common usage pattern to do eg map(stack) do array
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've meant to deprecate this for ages anyway. The worse cost is really how different splatting to a Tuple and a NamedTuple is.
Its a common pattern, but maplayers
should be fine to use for it, and frees up map
to later switch to work like iterate
(we have to error in between for a whole breaking change though).
@asinghvi17 this should do what you want
Closes #810