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

Add wrapper for eachslice? #25

Closed
rofinn opened this issue Sep 17, 2020 · 6 comments · Fixed by #27
Closed

Add wrapper for eachslice? #25

rofinn opened this issue Sep 17, 2020 · 6 comments · Fixed by #27

Comments

@rofinn
Copy link
Collaborator

rofinn commented Sep 17, 2020

I'm not sure what the best approach for this is, but it'd be nice if you could do:

eachslice(A; dims=[:channel])`

rather than

eachslice(A; dims=[dim(dimnames(A), :channel)])
@mcabbott
Copy link
Owner

Yes this ought to be made to work.

The catch is that you don't want to simply call eachslice(keyless(A), dims=d) after looking up d, as it would be nice to preserve the structure on each slice. But you can't dispatch on dims. So I guess you either copy-paste Base's definition, or you return another iterator which wraps each slice as it goes along.

@rafaqz
Copy link
Contributor

rafaqz commented Sep 17, 2020

FYI, I had this problem too, and had to copy-paste eachslice:

https://github.com/rafaqz/DimensionalData.jl/blob/master/src/methods.jl#L84-L97

It could be good to fix all these methods in base some day so we can dispatch properly on dims

@mcabbott
Copy link
Owner

Yes I tried to change NamedDims to that strategy, and the PR got lost in an argument about how to handle Julia < 1.1 most cleanly...

Re changing Base, I tried that too: JuliaLang/julia#32310 (comment)

@rafaqz
Copy link
Contributor

rafaqz commented Sep 17, 2020

Hmm no response in the Base PR. I had this too, ages ago:

https://github.com/JuliaLang/Statistics.jl/issues/11

But decided not to implement it yet as I wasn't sure I needed that many changes, and I don't in the end, I'm just dispatching on the array type instead of the dims type (I wanted to free up array inheritance for other things - ie dims are a trait, but its too hard).

eachslice and accumulate.jl are still problems I think.

I just didn't provide eachslice before 1.1. Will probably going to drop 1.0 support anyway soon, its starting to be a headache.

@mcabbott
Copy link
Owner

If someone was very energetic, I wonder what chance a PR adding a step via some to_dims(A, dims) to every single function in Base would have? Like to_index except only useful for things outside Base, I suppose.

@rafaqz
Copy link
Contributor

rafaqz commented Sep 17, 2020

That's a really clean way of doing it.

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 a pull request may close this issue.

3 participants