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

Support named dims for dims keyword argument #20

Closed
bencottier opened this issue Feb 19, 2021 · 5 comments · Fixed by #62
Closed

Support named dims for dims keyword argument #20

bencottier opened this issue Feb 19, 2021 · 5 comments · Fixed by #62
Assignees
Labels
enhancement New feature or request

Comments

@bencottier
Copy link
Contributor

To use a consistent convention for dims (#18), we currently have to "invert" the value passed into the dims keyword argument to the array-based apply!, so that eachslice gives the expected result. For example dims=2 becomes dims=1 and dims=3 becomes dims=[1, 2]. Basically we can use setdiff(1:ndims(array), dims) to accomplish that.

The solution isn't so straightforward for named dims, because the method to get the list of dim names is not universal. KeyedArray supports dimnames(A) but AxisArray does not, instead using axisnames. One workaround is dimnames(wrapdims(::AxisArray)) but that requires adding AxisKeys as a dependency.

Some have suggested that support for AxisArrays should be dropped entirely, or included in an auxiliary package - then we could rely on dimnames.

@glennmoy
Copy link
Member

"Not having named dims limits the usefulness of using a KeyedArray as opposed to just an AbstractArray"

@nicoleepp
Copy link
Contributor

I personally vote that adding the dependency on some of the underlying packages might be worth having named dims for

@bencottier
Copy link
Contributor Author

bencottier commented Mar 18, 2021

Are named dims actually prevented anywhere now? I think it may only be keys for inds now. I just checked named dims works for MeanStdScaling on KeyedArray.

@bencottier
Copy link
Contributor Author

I personally vote that adding the dependency on some of the underlying packages might be worth having named dims for

I don't have a good sense of the cost of adding the dependency. On the face of it, it seems worthwhile if we can't find any other non-hack way.

@glennmoy
Copy link
Member

Are named dims actually prevented anywhere now? I think it may only be keys for inds now. I just checked named dims works for MeanStdScaling on KeyedArray.

They are not in fact. We've added explicit tests in #62

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants