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

Test on dimnames for KeyedArrays #62

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Test on dimnames for KeyedArrays #62

merged 1 commit into from
Mar 23, 2021

Conversation

glennmoy
Copy link
Member

@glennmoy glennmoy commented Mar 22, 2021

Closes #20

I'll note that #20 describes the situation before we deprecated both eachslice (#15) and mapslices (#45) in the parent apply methods. So the problem it is describing is outdated since we no longer need to take the complement set of dimensions for any transformations.

Still it is important that we support dimnames since user feedback suggested "Not having named dims limits the usefulness of using a KeyedArray as opposed to just an AbstractArray". Fortunately, we already support this, this PR is just making that explicit via the tests.

However

  • You can't pass in the keys as inds (for now, we might support this in future)
  • This doesn't work with AxisArrays, at least not without AxisArrays first supporting selectdim. Considering we failed to get mapslices into AxisArrays I'm not in a rush to support it as we'll be dropping it internally altogether soon enough.

@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #62 (6f67563) into main (ab6ab74) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #62   +/-   ##
=======================================
  Coverage   99.04%   99.04%           
=======================================
  Files          10       10           
  Lines         105      105           
=======================================
  Hits          104      104           
  Misses          1        1           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab6ab74...6f67563. Read the comment docs.

Copy link
Contributor

@bencottier bencottier left a comment

Choose a reason for hiding this comment

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

Looks good to me

@glennmoy glennmoy merged commit 0194507 into main Mar 23, 2021
@glennmoy glennmoy deleted the gm/test_dimnames branch June 21, 2021 12:58
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.

Support named dims for dims keyword argument
2 participants